-
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
Make SliceTransform into a Customizable class #8641
Conversation
|
||
const char* Name() const override { return name_.c_str(); } |
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.
Is there any backward compatibility issue when Name() is used here. In the legacy code, we will get "rocksdb.CappedPrefix.(cap_len)" But now, we only have rocksdb.CappedPrefix
. I'm not sure if user will change them to GetId()
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.
I do not think this will be an issue. First, I believe the Name() method was primarily used for Dump and related functions. I did miss one place in Dump that I updated and will update HISTORY with this change.
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.
In general, it looks good
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
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.
LGTM, do remember to mention the change in HISTORY.md
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
@mrambacher has updated the pull request. You must reimport the pull request before landing. |
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
1 similar comment
@mrambacher has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Summary: Made SliceTransform into a Customizable class. Would be nice to write a test that stored and used a custom transform in an SST table. There are a set of tests (DBBlockFliterTest.PrefixExtractor*, SamePrefixTest.InDomainTest, PrefixTest.PrefixAndWholeKeyTest that run the same with or without a SliceTransform/PrefixFilter. Is this expected? Pull Request resolved: facebook/rocksdb#8641 Reviewed By: zhichao-cao Differential Revision: D31142793 Pulled By: mrambacher fbshipit-source-id: bb08672fccbfdc263dcae21f25a62307e1facda1
Made SliceTransform into a Customizable class.
Would be nice to write a test that stored and used a custom transform in an SST table.
There are a set of tests (DBBlockFliterTest.PrefixExtractor*, SamePrefixTest.InDomainTest, PrefixTest.PrefixAndWholeKeyTest that run the same with or without a SliceTransform/PrefixFilter. Is this expected?