[Backport stable/8.4] Avoid accidental reordering of column family values #16129
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This doesn't change any behavior. We introduce a new EnumValue interface that can be implemented by Enums to avoid using ordinal as the value associated with each enum value. We also implement this in the ZbColumnFamilies enum as this suffers from bugs caused by ordinal re-ordering. While this doesn't yet move ZbColumnFamilies away from using an ordinal (note the implementation), it already prepares the enum for moving to a hardcoded number system. We'll add this number system in a later commit. (cherry picked from commit 281c321) Tried to add not yet supported column family: COMPENSATION_SUBSCRIPTION Conflicts: protocol/src/main/java/io/camunda/zeebe/protocol/ZbColumnFamilies.java
This is only to have access to EnumValue inside of zb-db. Alternatives considered: - add EnumValue to zeebe-util, would require zeebe-protocol to depend on zeebe-util - move ZbColumnFamilies/EnumValue to a different module, but it's not clear where. Likely, a good place is zeebe-state (when we finally add that module), which would clarify which modules depend on the state (backup, engine, etc). Note that ZbColumnFamilies used to be in the engine module, but was moved out when splitting the engine and stream processor. For now, this doesn't seem like a solution. (cherry picked from commit a695a5f)
We're introducing EnumValue as a way to associate hardcoded values with the ZbColumnFamilies rather than the currently used ordinals. zb-db doesn't depend on ZbColumnFamilies directly, so it can function regardless of the data that is stored in it. This makes sense, because the data schema is decided by modules like engine (in the state package). To allow using EnumValue's .getValue() in subsequent commits inside the zb-db module, we need to clarify some structure for the ColumnFamilyType generics: it must be an enum that implements EnumValue. We can achieve that by combining some generics. First, we say that the type extends an Enum that itself extends EnumValue. However, then we don't have access to the .getValue() method yet. To add that, we also have to say that the type extends EnumValue itself. This combination can be achieved with Intersection Types (A & B). Note that for the test column family enums, it doesn't matter how the getValue method is implemented. We can use .ordinal() in the test classes, because they are not used in production code. While, DefaultColumnFamily is also not used in production code, it does exist in `main` and could be used in production later. Therefore, I've implemented that getValue with an actual hardcoded value. (cherry picked from commit 7794eae)
This adds two tests. The main test is shouldNotReuseEnumValues. It ensures that the getValue() method produces a unique value for each value in the enum. It does not test determinism, but that is implied for this method. The current implementation uses the ordinal() method. To reimplement it, I've added a temporary test that we can remove after re-implementing the getValue() method. It should pass after re-implementation to ensure we don't accidentally reorder the values now. (cherry picked from commit e415790)
While this is a refactor: it does not change behavior, it strengthens our ability to add and backport new column families by hardcoding the associated value. A test exists that verifies that all hardcoded values are added using the exact same value as the ordinal currently has. Another test exists that verifies that no hardcoded value is reused. This one is especially important, as it protects against accidental problems while automatically backporting. After this change, values don't necessarily need to be added to the bottom anymore, as long as they receive a unique value. (cherry picked from commit 9118269) Tried to add not yet supported column family COMPENSATION_SUBSCRIPTION(81). Conflicts: protocol/src/main/java/io/camunda/zeebe/protocol/ZbColumnFamilies.java
This test case is no longer needed. From now on, the ZbColumnFamilies values must remain the same over time (we have hardcoded them) and must be unique (there is another test case that checks this). But there is no need anymore for the values to match exactly the ordinals. (cherry picked from commit 759d3b0)
Rather than using the ordinal, we can now use the static enum value of the column family. The value is/was used to determine the prefix of keys in the column family. (cherry picked from commit 36e85ec)
As database entries are stored with the column family's value prefixed to the key, so too do we look them up with that value when checking foreign key constraints. We assume that the foreignKey.columnFamily() returns an instance of EnumValue so we can call the .getValue() on it. This is safe to do because the column family must anyways be an instance extending Enum<? extends EnumValue>. Alternatively, we could've avoided this cast if we would make the foreign key understand that the column family type also actually implements EnumValue, using an intersection type. However, that would've required adding this intersection type as an additional generic on the DbForeignKey<DbKey, CFType>. As this class is used often in our state classes, it would deteriorate the readability of those classes, as we currently can refer to it as DbForeignKey<DbKey>. This is why I went for casting rather than more generics. (cherry picked from commit aa61c8b)
(cherry picked from commit a1e03a5)
This makes it almost impossible to make mistakes, because we can no longer accidentally skip an enum value. (cherry picked from commit fb9bab0)
Zelldon
approved these changes
Jan 31, 2024
|
|
||
| USER_TASKS, | ||
| USER_TASK_STATES, |
There was a problem hiding this comment.
❓ 🤔 I somehow thought/expected that we backport all CFs now, for example https://github.com/camunda/zeebe/blob/main/protocol/src/main/java/io/camunda/zeebe/protocol/ZbColumnFamilies.java#L175 is missing.
There was a problem hiding this comment.
I considered this, but the problem is that it would require fixing the bug in one go on 8.1.
If we would just copy the current CFs file to 8.1, then we also need to add the recovery migration task. Otherwise we're now losing the data on 8.1.
I wanted to split it into two steps:
- first add some safety guards
- then add the migration task to the older versions
There was a problem hiding this comment.
I think this was not clear to me.
There was a problem hiding this comment.
Does it make sense or do you think I should change the approach and do it all at once?
There was a problem hiding this comment.
I think we can also do it like this, but should not forget to do it.
Zelldon
approved these changes
Jan 31, 2024
Zelldon
approved these changes
Jan 31, 2024
This was referenced Jan 31, 2024
14 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Backport of #16106 to
stable/8.4.Relates to #15804
Due to conflicts, I would recommend you review the commits individually. Commits that resolve conflicts mention this at the bottom of the commit message (under the
cherry picked from commit xline).There were conflicts because the following column family does not exist on 8.4 yet:
COMPENSATION_SUBSCRIPTION(81)