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

chore(zb-db): introduce RocksDBConfig and made statistics optional #6246

Merged
2 commits merged into from
Feb 3, 2021

Conversation

Zelldon
Copy link
Member

@Zelldon Zelldon commented Feb 2, 2021

Description

  • Introduce the RocksDbConfiguration class which can be used with the ZeebeDbFactory.
  • Made Statistics optional
  • Remove unused Enum class object

Related issues

closes #6157

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 to the last two minor versions. You can trigger a backport by assigning labels (e.g. backport stable/0.25) to the PR, in case that fails you need to create backports manually.

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
  • The change has been verified by a QA run
  • The impact of the changes is verified by a benchmark

Documentation:

  • The documentation is updated (e.g. BPMN reference, configuration, examples, get-started guides, etc.)
  • New content is added to the release announcement

@Zelldon Zelldon requested a review from npepinpe February 2, 2021 06:26
Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

👍

This means the ColumnFamilyType of the ZeebeDbFactory is now inferred via return type? But then the DB it produces will have the right type. I don't see any downsides, I guess - I'm still thinking if we should keep the enums in general, but we can debate that later.

.setLogFileTimeToRoll(Duration.ofMinutes(30).toSeconds())
.setKeepLogFileNum(2)
// can be disabled when not profiling
.setStatsDumpPeriodSec(20);
Copy link
Member

Choose a reason for hiding this comment

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

this should also be set only if statistics is enabled, or?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah makes probably sense 👍

@Zelldon
Copy link
Member Author

Zelldon commented Feb 3, 2021

This means the ColumnFamilyType of the ZeebeDbFactory is now inferred via return type? But then the DB it produces will have the right type. I don't see any downsides, I guess - I'm still thinking if we should keep the enums in general, but we can debate that later.

You can create an follow up issue if you see a need. I think it makes it harder to use, if we not have the enum especially how will you differentiate between the different types to make sure that you are not iterating over variables and messages when searching for a job etc.

@Zelldon
Copy link
Member Author

Zelldon commented Feb 3, 2021

@npepinpe I updated the PR please have a look

Copy link
Member

@npepinpe npepinpe 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!

@Zelldon
Copy link
Member Author

Zelldon commented Feb 3, 2021

Thanks @npepinpe and now get some rest :)

@Zelldon
Copy link
Member Author

Zelldon commented Feb 3, 2021

bors r+

@ghost
Copy link

ghost commented Feb 3, 2021

Build succeeded:

@ghost ghost merged commit ac4e4d4 into develop Feb 3, 2021
@ghost ghost deleted the zell-6157-statistics branch February 3, 2021 08:38
This pull request was closed.
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.

Make RocksDB statistics optional
2 participants