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 a bug of not setting enforce_single_del_contracts #10027

Closed

Conversation

riversand963
Copy link
Contributor

@riversand963 riversand963 commented May 20, 2022

Summary:
Before this PR, BuildDBOptions() does not set a newly-added option, i.e.
enforce_single_del_contracts, causing OPTIONS files to contain incorrect
information.

Test Plan:
make check
Manually check OPTIONS file.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@riversand963
Copy link
Contributor Author

I am not entirely happy with the test plan, because I would prefer a test which will fail if a similar bug happens in the future.

@mrambacher
Copy link
Contributor

I am not entirely happy with the test plan, because I would prefer a test which will fail if a similar bug happens in the future.

There is already a test that builds options from Immutable/MutableOptions (OptionsTest::DBOptionsComposeImmutable). One issue is that the new option is not part of the randomized values in RandomInitDBOptions. It might also be useful if this specific test did multiple runs to allow different random values.

@facebook-github-bot
Copy link
Contributor

@riversand963 has updated the pull request. You must reimport the pull request before landing.

Summary:
Before this PR, BuildDBOptions() does not set a newly-added option, i.e.
enforce_single_del_contracts, causing OPTIONS files to contain incorrect
information.

Test Plan:
make check
@facebook-github-bot
Copy link
Contributor

@riversand963 has updated the pull request. You must reimport the pull request before landing.

@riversand963
Copy link
Contributor Author

I am not entirely happy with the test plan, because I would prefer a test which will fail if a similar bug happens in the future.

There is already a test that builds options from Immutable/MutableOptions (OptionsTest::DBOptionsComposeImmutable). One issue is that the new option is not part of the randomized values in RandomInitDBOptions. It might also be useful if this specific test did multiple runs to allow different random values.

Added to the test. Thanks for the info. My knowledge about options setting should be updated.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@riversand963 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

This reverts commit 9128a28.
@facebook-github-bot
Copy link
Contributor

@riversand963 has updated the pull request. You must reimport the pull request before landing.

@facebook-github-bot
Copy link
Contributor

@riversand963 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@riversand963 riversand963 requested a review from ltamasi May 20, 2022 22:51
@riversand963 riversand963 deleted the fix-single-del-enforce-opts branch May 21, 2022 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants