[stable/8.2] Allow decision requirements in column family 49 #16411
Conversation
korthout
changed the title
This test case was not surfacing a bug due to usage of a specific user-defined string as the decisionRequirementsId. This meant that the correct data did not trigger the false positive where it would be attempted to be moved.
We need to be able to count the entries in the column family regardless of whether they can be deserialized. To do so, I copied these methods from main.
This count method avoids deserialization and can thus verify incorrect data
korthout
force-pushed
the
korthout-16406-allow-drgs-in-cf-49
branch
from
f9f6ca1
to
87a08e9
Compare
Previously, this code tried to move the data if it did not match the processInstanceKeyByProcessDefinitionKey but actually it should move it when it does match that. another way to look at it, is that it should leave entries that match the decisionRequirementsIdAndVersion key should be left alone because they belong in this column family. Only entries that cannot be matched to the decisionRequirementsIdAndVersion should be moved.
Adds another assertion to verify that the data that is not moved by the corrector is the entries that we expect to stay in the column family (i.e. the correct data)
korthout
force-pushed
the
korthout-16406-allow-drgs-in-cf-49
branch
from
87a08e9
to
94a79c7
Compare
zb-db/src/main/java/io/camunda/zeebe/db/impl/rocksdb/transaction/TransactionalColumnFamily.java
Show resolved
Hide resolved
...java/io/camunda/zeebe/engine/state/migration/to_8_2/corrections/ColumnFamily49Corrector.java
Show resolved
Hide resolved
| @@ -272,7 +272,7 @@ void shouldMovePiKeyByProcDefKeyToCorrectColumnFamily() { | |||
| @Test | |||
| void shouldIgnoreProcessInstanceKeyByDefinitionKeyEntries() { | |||
| // given | |||
| decisionRequirementsId.wrapString("decisionRequirements"); | |||
There was a problem hiding this comment.
❓ Is this because the length is too large and it would catch it already before (on the length)?
There was a problem hiding this comment.
If this is the case, could we still have both? So we test all paths?
There was a problem hiding this comment.
No, it reaches the faulty wrap method here, but it would still allow wrapping and not throw an exception
There was a problem hiding this comment.
@Zelldon Do you think we need another resolution? Or shall I merge?
There was a problem hiding this comment.
If this is the case, could we still have both? So we test all paths?
Well, we could do that 👍
There was a problem hiding this comment.
How 🤔 processInstanceKeyByProcessDefinitionKey is a Long + Long ? So 16 length🤔 decisionRequirements would be 20 chars + the length in front. I'm wondering why this works.
There was a problem hiding this comment.
Wrapping is limited by a maximum, but not an exact size. So, if it can fit the first 16 bytes as two longs than it's fine
There was a problem hiding this comment.
By the way, I tried with the original string, and that passed as well. I don't want to add it now, because it doesn't matter and slows down the release. Let's just merge this.
|
Backport failed for Please cherry-pick the changes locally. git fetch origin stable/8.1
git worktree add -d .worktree/backport-16411-to-stable/8.1 origin/stable/8.1
cd .worktree/backport-16411-to-stable/8.1
git checkout -b backport-16411-to-stable/8.1
ancref=$(git merge-base a6bf0e89de97a828888476876466f21b6b32eb5b 94a79c727e6cd6bee66b6a084c7a6195b1b1d562)
git cherry-pick -x $ancref..94a79c727e6cd6bee66b6a084c7a6195b1b1d562 |
megglos
added
the
version:8.2.23
Description
Fixes a bug in
ColumnFamily49Correctorwhere it falsely detected a correct DRG entry in column family 49 as an entry that should be moved toPROCESS_INSTANCE_KEY_BY_DEFINITION_KEY.Changes have been tested manually locally against the data folder of the affected cluster.
Related issues
closes #16406
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
backport stable/1.3) to the PR, in case that fails you need to create backports manually.Testing:
Documentation:
Other teams:
If the change impacts another team an issue has been created for this team, explaining what they need to do to support this change.
Please refer to our review guidelines.