Add rocksdb_create_column_families_with_options to C API. #12896
Conversation
db/c_test.c
Outdated
| CheckNoError(err); | ||
|
|
||
| char** list_const_cf_names = (char**)malloc(2 * sizeof(char*)); | ||
| list_const_cf_names[0] = "cf_with_options"; |
There was a problem hiding this comment.
Could you make the names more distinct ?
db/c_test.c
Outdated
| list_const_cf_names[0] = "cf_with_options"; | ||
| list_const_cf_names[1] = "cf_with_option"; | ||
| size_t cflen; | ||
| const rocksdb_options_t* create_cf_options[2] = {db_options, db_options}; |
There was a problem hiding this comment.
Could you
- create the 2x CF with different options
- do basic write/read tests just to validate that the correct distinct CFs have been returned by the new API
- confirm that each has received these distinct options i.e. something in the use of CF that differs by option
Maybe some of this can be copied from the tests that exist already ?
There was a problem hiding this comment.
Thanks for expanding the tests. Is there anything that demonstrates the correct (different) options have been sent to the different CFs ?
There was a problem hiding this comment.
Apparently get_options(cfh) is not implemented in the 'C' API. We really should add it, and use it to verify the correct options go to the correct CF on this creation. In the meantime, let's proceed with this PR.
| @@ -1145,6 +1145,34 @@ rocksdb_column_family_handle_t** rocksdb_create_column_families( | |||
| return c_handles; | |||
| } | |||
|
|
|||
| rocksdb_column_family_handle_t** rocksdb_create_column_families_with_options( | |||
There was a problem hiding this comment.
I approve of changing the parameter order so as not to allow confusion of options[] and options* ...
There was a problem hiding this comment.
Sorry @alanpaxton, I don't understand what are you saying. Can you please describe it more?
There was a problem hiding this comment.
There are different rocksdb_create_column_famil[y|ies] methods with different orders of parameters. I was confused by the different orders of parameters. But I think your function has the "correct" order of parameters for what it does.
| @@ -1145,6 +1145,34 @@ rocksdb_column_family_handle_t** rocksdb_create_column_families( | |||
| return c_handles; | |||
| } | |||
|
|
|||
| rocksdb_column_family_handle_t** rocksdb_create_column_families_with_options( | |||
There was a problem hiding this comment.
I know the other APIs don't have method comments, but maybe we could start ? In particular state that this method takes a set of options per CF.
There was a problem hiding this comment.
Yes, but wasn't be better to add doc comments to c.h file?
There was a problem hiding this comment.
You are correct, at least in this case where we are documenting the API. Thanks for doing that.
| @@ -1145,6 +1145,34 @@ rocksdb_column_family_handle_t** rocksdb_create_column_families( | |||
| return c_handles; | |||
| } | |||
|
|
|||
| rocksdb_column_family_handle_t** rocksdb_create_column_families_with_options( | |||
There was a problem hiding this comment.
Should we implement a TTL variant, or is that enough scope ? I prefer to restrict the scope but worth thinking about.
There was a problem hiding this comment.
This is the first C API. I would like to start small and then we can add more later. Once API is out, it's hard to remove it.
rhubner
force-pushed
the
eb/cf-with-options
branch
2 times, most recently
from
5e7039e
to
e134e79
Compare
rhubner
force-pushed
the
eb/cf-with-options
branch
from
e134e79
to
7ac10a4
Compare
rhubner
force-pushed
the
eb/cf-with-options
branch
from
7ac10a4
to
1d3aeda
Compare
rhubner
force-pushed
the
eb/cf-with-options
branch
from
1d3aeda
to
40b5fb6
Compare
This method allow to create Column Families in same was as we do in C++ with ColumnFamilyDescriptor.
Fix #12887