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

Use the correct default values for compaction group configs #12937

Open
hzxa21 opened this issue Oct 18, 2023 · 3 comments
Open

Use the correct default values for compaction group configs #12937

hzxa21 opened this issue Oct 18, 2023 · 3 comments

Comments

@hzxa21
Copy link
Collaborator

hzxa21 commented Oct 18, 2023

Currently compaction group configs are persisted in meta store and will be restored on meta restart. We have met issues related to how default values are determined:

  • For newly added configs, we currently cannot differentiate default value and missing values. For example, the newly added enable_emergecy_picker and tombstone_recliam_ratio configs on existing compaction groups will be interpreted as false and 0, which can cause unexpected behavior after version upgrade.
  • For configs with changed default values, we currently cannot handle as well because we cannot differentiate whether user has explicitly configured the field. For example, the default value of level0_stop_write_threshold_sub_level_number has changed from 1000 to 300 but existing compaction groups won't adopt this default value changes even though user hasn't explicitly configured it.

The solution is quite simple: we should use the optional keyword in compaction group config proto so that we can perform presence check

  • If a field is none (absent), use default value
  • If a field is set, use the persisted value
@github-actions github-actions bot added this to the release-1.4 milestone Oct 18, 2023
@hzxa21
Copy link
Collaborator Author

hzxa21 commented Oct 18, 2023

For newly added fields, we can use optional directly.

For existing fields that use default values, maybe we can do the change altogether with the SQL meta store backend migration.

@hzxa21 hzxa21 modified the milestones: release-1.4, release-1.5 Nov 8, 2023
@hzxa21 hzxa21 self-assigned this Nov 8, 2023
@hzxa21
Copy link
Collaborator Author

hzxa21 commented Dec 4, 2023

This is more like a guideline instead of a working item. I will remove it from milestone.

@hzxa21 hzxa21 removed their assignment Dec 4, 2023
@hzxa21 hzxa21 removed this from the release-1.5 milestone Dec 4, 2023
Copy link
Contributor

This issue has been open for 60 days with no activity. Could you please update the status? Feel free to continue discussion or close as not planned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant