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

Remove BlockBasedTableOptions.hash_index_allow_collision #9454

Closed

Conversation

siying
Copy link
Contributor

@siying siying commented Jan 26, 2022

Summary:
BlockBasedTableOptions.hash_index_allow_collision is already deprecated and has no effect. Delete it for preparing 7.0 release.

Test Plan: Run all existing tests.

@facebook-github-bot
Copy link
Contributor

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

Copy link
Contributor

@ajkr ajkr left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +875 to +877
// rocksdb_block_based_options_set_hash_index_allow_collision()
// is removed since BlockBasedTableOptions.hash_index_allow_collision()
// is removed
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see such a notice in other removal PRs but am fine with having it. Although I didn't strongly feel it's needed so don't plan to push others to add such notice (if you do, you may wish to check all the other removal PRs that have been approved/landed lately).

Copy link
Contributor

@pdillinger pdillinger Feb 10, 2022

Choose a reason for hiding this comment

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

If the current public API is consistent with itself, there should be no reason to talk in API docs about what used to be. History is for HISTORY.md, git log, etc.

@siying siying force-pushed the remove_hash_index_allow_collision branch from 68f3b7d to aaf503f Compare January 27, 2022 22:40
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@siying siying force-pushed the remove_hash_index_allow_collision branch from aaf503f to 03ecaad Compare February 2, 2022 21:22
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

@siying siying force-pushed the remove_hash_index_allow_collision branch from 03ecaad to b61e0f8 Compare February 2, 2022 21:49
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

Summary:
BlockBasedTableOptions.hash_index_allow_collision is already deprecated and has no effect. Delete it for preparing 7.0 release.

Test Plan: Run all existing tests.
@siying siying force-pushed the remove_hash_index_allow_collision branch from b61e0f8 to 7d99f9f Compare March 1, 2022 19:38
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

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