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

fix(config): wrong rocksdb config name max_sub_compactions #2499

Merged
merged 1 commit into from
Aug 21, 2024

Conversation

ChrisZMF
Copy link
Contributor

Configuration item rocksdb.max_sub_compactions should be rocksdb.max_subcompactions.
Try set rocksdb.max_sub_compactions:
image
Fix it as rocksdb.max_subcompactions, try again:
image

@ChrisZMF ChrisZMF requested a review from PragmaTwice August 20, 2024 12:39
tisonkun
tisonkun previously approved these changes Aug 20, 2024
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. But seems no regression tests to guard this manner ..

Copy link
Member

@git-hulk git-hulk left a comment

Choose a reason for hiding this comment

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

LGTM, good catch.

@git-hulk git-hulk requested a review from tisonkun August 20, 2024 13:07
@git-hulk git-hulk changed the title correct rocksdb.max_sub_compactions fix(config): incorrect rocksdb max_sub_compactions Aug 20, 2024
@git-hulk git-hulk changed the title fix(config): incorrect rocksdb max_sub_compactions fix(config): wrong rocksdb config name max_sub_compactions Aug 20, 2024
Copy link

@git-hulk git-hulk merged commit afd0a1b into apache:unstable Aug 21, 2024
33 checks passed
@ChrisZMF ChrisZMF deleted the fix-max-subcompactions-dev branch August 21, 2024 01:56
@ChrisZMF
Copy link
Contributor Author

LGTM. But seems no regression tests to guard this manner ..

Unit tests can cover this scenario. The reason why unit test was ineffective before is that callback was not registered for the item, which means config set command did not actually execute in rocksdb. See
image

@mapleFU
Copy link
Member

mapleFU commented Aug 21, 2024

We can first checkin, and adding a cfg testing, and everyone we add a new config, a go-side cfg test should be added to cover the case?

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.

4 participants