-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
Disable snapshot refresh feature when snap_refresh_nanos is 0 #5724
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
79a7ec5
to
22b26e0
Compare
@maysamyabandeh has updated the pull request. Re-import the pull request |
22b26e0
to
c57334f
Compare
@maysamyabandeh has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
I didn't see "test plan" in PR summary. What's your test plan? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change is OK, but we need a convincing test plan. Maybe a unit test scenario.
The added assert statement is the test plan. It caught the existing bug and also another one in the unit tests where snap_refresh_time_ was set to zero, interpreting that as no delay instead of disabled feature. |
@maysamyabandeh OK then. |
@maysamyabandeh merged this pull request in 7bc18e2. |
Summary: The comments of snap_refresh_nanos advertise that the snapshot refresh feature will be disabled when the option is set to 0. This contract is however not honored in the code: #5278 The patch fixes that and also adds an assert to ensure that the feature is not used when the option is zero. Pull Request resolved: #5724 Differential Revision: D16918185 Pulled By: maysamyabandeh fbshipit-source-id: fec167287df7d85093e087fc39c0eb243e3bbd7e
Summary: The comments of snap_refresh_nanos advertise that the snapshot refresh feature will be disabled when the option is set to 0. This contract is however not honored in the code: #5278 The patch fixes that and also adds an assert to ensure that the feature is not used when the option is zero. Pull Request resolved: #5724 Differential Revision: D16918185 Pulled By: maysamyabandeh fbshipit-source-id: fec167287df7d85093e087fc39c0eb243e3bbd7e
Summary: The comments of snap_refresh_nanos advertise that the snapshot refresh feature will be disabled when the option is set to 0. This contract is however not honored in the code: #5278 The patch fixes that and also adds an assert to ensure that the feature is not used when the option is zero. Pull Request resolved: #5724 Differential Revision: D16918185 Pulled By: maysamyabandeh fbshipit-source-id: fec167287df7d85093e087fc39c0eb243e3bbd7e
Summary: The comments of snap_refresh_nanos advertise that the snapshot refresh feature will be disabled when the option is set to 0. This contract is however not honored in the code: #5278 The patch fixes that and also adds an assert to ensure that the feature is not used when the option is zero. Pull Request resolved: #5724 Differential Revision: D16918185 Pulled By: maysamyabandeh fbshipit-source-id: fec167287df7d85093e087fc39c0eb243e3bbd7e
Revert "Allow users to stop manual compactions (facebook#3971)" This reverts commit 6226830. Deprecate snap_refresh_nanos feature Revert "Disable snapshot refresh feature when snap_refresh_nanos is 0 (facebook#5724)" This reverts commit dd757ff. Revert "Declare snapshot refresh incompatible with delete range (facebook#5625)" This reverts commit fdc56e2. Revert "Disable refresh snapshot feature by default (facebook#5606)" This reverts commit cfd6eb3. Revert snapshot refresh feature deprecate the option Revert "Revert "Allow users to stop manual compactions (facebook#3971)"" This reverts commit 6194c8f70b141899f66d777a7b1749ac346770c4.
…ok#5724) Summary: The comments of snap_refresh_nanos advertise that the snapshot refresh feature will be disabled when the option is set to 0. This contract is however not honored in the code: facebook#5278 The patch fixes that and also adds an assert to ensure that the feature is not used when the option is zero. Pull Request resolved: facebook#5724 Differential Revision: D16918185 Pulled By: maysamyabandeh fbshipit-source-id: fec167287df7d85093e087fc39c0eb243e3bbd7e
The comments of snap_refresh_nanos advertise that the snapshot refresh feature will be disabled when the option is set to 0. This contract is however not honored in the code: #5278
The patch fixes that and also adds an assert to ensure that the feature is not used when the option is zero.