Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump Zeebe to version 8.0.0-rc1 #264

Merged
merged 2 commits into from
Mar 29, 2022
Merged

Conversation

remcowesterhoud
Copy link
Contributor

@remcowesterhoud remcowesterhoud commented Mar 24, 2022

Description

The ColumnFamily interface in Zeebe has been modified. Before it only contained a put method. Starting from version 8.0.0-rc1 this has been replaced with update, insert and upsert methods.
The delete method has been replaced with deleteExisting and deleteIfExists.

In our in memory database we implement this interface. When updating to Zeebe 8.0.0-rc1 these changed need to be merged to keep the code from breaking.

In addition to the ColumnFamily changes the zeebe-protocol-jackson changed in the way we need to do the mapping. This minor mapping change has also been fixed in this PR.

Related issues

closes #262

Definition of Done

Not all items need to be done depending on the issue and the pull request.

Code changes:

  • The changes are backwards compatibility with previous versions
  • If it fixes a bug then PRs are created to backport the fix

Testing:

  • There are unit/integration tests that verify all acceptance criterias of the issue
  • New tests are written to ensure backwards compatibility with further versions
  • The behavior is tested manually

Documentation:

  • Javadoc has been written
  • The documentation is updated

@remcowesterhoud remcowesterhoud requested a review from pihme March 24, 2022 15:19
@remcowesterhoud remcowesterhoud changed the title Implement ColumnFamily API changes Bump Zeebe to version 8.0.0-rc1 Mar 29, 2022
@github-actions
Copy link

github-actions bot commented Mar 29, 2022

Unit Test Results

132 files  132 suites   6m 14s ⏱️
232 tests 232 ✔️ 0 💤 0
896 runs  896 ✔️ 0 💤 0

Results for commit f79df18.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@pihme pihme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hard one. The changes look ok. But now the behavior of InMemoryEngine differs from Zeebe. In Zeebe the consistency checks are governed by feature flags. Currently the checks are disabled by default.

The tricky bit is that we don't have any notion of feature flags yet in the InMemoryEngine. And maybe we should not? I am torn on this one, to be honest.

I also wonder whether the foreign key checks are in place because they happen on another level, or whether we have to reimplement those as well.

I see two ways going forward:

  • we allow to pass in a configuration object and reimplement the relevant feature flags here
  • we purposefully deviate from the behavior of Zeebe, but then I would at least try to mimick the default configuration, which currently is with integrity checks disabled.

There should also be tests in Zeebe for behavior of this class with feature flags enabled vs disabled. Maybe we should copy those as well.

@remcowesterhoud
Copy link
Contributor Author

remcowesterhoud commented Mar 29, 2022

After some discussion we have decided to keep the changes as they are at this moment. This means we will check for existing keys where relevant, and we will not check for foreign keys.

The reasoning behind this decision is that the ForeignKeyChecker in Zeebe is not reusable in this project as is. If we would add this consistency check it would involve creating it ourselves. This is doable. However, with creating our own implementation we are already deviating from Zeebe. Doing this will raise the question what the value of this check is. It will differ from the Zeebe implementation and it is not something users of this project will require us to have. Any conflict on our foreign key check will be short-lived as the db will be reset after each test case anyways. Worst-case a test will fail.

For this reason we have decided to deviate from the Zeebe database. We aim to keep the databases as similar as possible, but this is not always a feasible option.

Copy link
Contributor

@pihme pihme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

two minor suggestions

The ColumnFamily interface in Zeebe has been modified. Before it only contained a put method. Starting from version 8.0.0 this has been replaced with update, insert and upsert methods.
The delete method has been replaced with deleteExisting and deleteIfExists.

In our in memory database we implement this interface. When updating to Zeebe 8.0.0 these changed need to be merged to keep the code from breaking.
The api changes to ColumnFamily are first available in Zeebe 8.0.0-rc1. We need to use this version to create our own release candidate.
Upon upgrading I found there was another breaking change made in zeebe-protocol-jackson. For this reason a small change has been made to the way we do the mapping of records in ContainerizedEngine.
@remcowesterhoud remcowesterhoud force-pushed the 262-columnfamily_changes branch from 26ec1e8 to f79df18 Compare March 29, 2022 13:41
@remcowesterhoud remcowesterhoud merged commit b169b92 into main Mar 29, 2022
@remcowesterhoud remcowesterhoud deleted the 262-columnfamily_changes branch March 29, 2022 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adjust ColumnFamily implementation to reflect API changes in Zeebe
2 participants