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
add c-api to get default cf handle #12514
base: main
Are you sure you want to change the base?
Conversation
@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
void rocksdb_default_column_family_handle_destroy( | ||
rocksdb_column_family_handle_t* handle) { |
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.
It's kind of confusing because (1) it has "destroy" in the name but doesn't go through our regular destroy path (DestroyColumnFamilyHandle()
or ~ColumnFamilyHandle()
); and (2) it has "default" in the name but operates on any rocksdb_column_family_handle_t*
.
What do you think about simply not offering such an API, and let the user call delete handle
instead?
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 reason I add this different api is that, I observed that "delete handle" api will destroy the cf together with the handle, for other cf this behavior is rational because except for the handle, user has no other way to get and work on it, free the handle should also free the cf, too.
But for the default cf, things have a little change, only the new batched multi_get api need the handle arg, what if I finish the call and just want to free the memory of default cf_handle? Maybe another api name is more suitable, what do you think?
Or we can modify the batched multi_get api, if the cf_handle arg is NULL, actions will go to the default cf by default? Maybe adding a new api "rocksdb_batched_multi_get" which use the default cf is more direct, as all other APIs have two version -- with or without "_cf" ending, except for batched multi_get.
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.
Oh ok I see the ambiguity of the term "destroy" in our C API is preexisting. How about this as the API change?
struct rocksdb_column_family_handle_t {
ColumnFamilyHandle* rep;
bool immortal; // <-- New field
};
extern ROCKSDB_LIBRARY_API rocksdb_column_family_handle_t*
rocksdb_get_default_column_family_handle(rocksdb_t* db); // <-- New function
rocksdb_get_default_column_family_handle()
will setimmortal == true
- Other creations of
rocksdb_column_family_handle_t
, whererep
is not obtained byDefaultColumnFamilyHandle()
, will setimmortal == false
rocksdb_column_family_handle_destroy()
will deleterep
only when!immortal
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.
OK, that seems rational, thank you.
Can I expect to see this new API in the next release?
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.
Yes, we will be able to include it in the 9.2 release as long as we land it by Friday this week.
rocksdb_batched_multi_get_cf has performance improvement than normal multi_get, however it needs a cf_handle arg, so add a C-API to get and destroy the default cf_handle, as many user only use the default cf.
Fixes #12316