Add a new SstFileWriter constructor without explicit comparator #1978
Conversation
|
@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
I'll try fixing the test failures, and send out a new revision for review soon. |
|
@sagar0 updated the pull request - view changes - changes since last import |
|
@sagar0 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| return writer; | ||
| } | ||
|
|
||
| rocksdb_sstfilewriter_t* rocksdb_sstfilewriter_create_with_comparator( | ||
| const rocksdb_envoptions_t* env, const rocksdb_options_t* io_options, | ||
| const rocksdb_comparator_t* comparator) { | ||
| rocksdb_sstfilewriter_t* writer = new rocksdb_sstfilewriter_t; | ||
| writer->rep = | ||
| new SstFileWriter(env->rep, io_options->rep, io_options->rep.comparator); | ||
| writer->rep = new SstFileWriter(env->rep, io_options->rep); |
There was a problem hiding this comment.
let's not change this since we did not change the API
There was a problem hiding this comment.
The public API in c.h cannot be changed, but, just curious, what's your reasoning for not changing the implementation here?
As it stands today, even if someone passes a new comparator arg, we seem to be just discarding it and using io_options->rep.comparator anyway.
There was a problem hiding this comment.
Oh yes, I did not recognize that we are already ignoring the passed comparator
The comparator param in SstFileWriter constructor is redundant as it already exists as a field in options. So the current SstFileWriter constructor should be deprecated in favor of a new one which does not take a comparator.
Note that the jni/java apis have not been touched yet.
Test Plan: Did a make check to make sure that nothing's broken.