Unbreak MemTableRep API change #3513
Conversation
There was a problem hiding this comment.
@maysamyabandeh has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
include/rocksdb/memtablerep.h
Outdated
| // Returns false if MemTableRepFactory::CanHandleDuplicatedKey() is true and | ||
| // the <key, seq> already exists. | ||
| virtual bool Insert(KeyHandle handle) = 0; | ||
| virtual bool InsertAndReturn(KeyHandle handle) { |
There was a problem hiding this comment.
s/InsertAndReturn/InsertUnique/ and same for the rest?
There was a problem hiding this comment.
I like the *Unique idea but I am concerned that i would cause confusion as *Unique could return true even if the underlying structure does not support checking for uniqueness. And I would like to avoid the overhead of a if-then-else branch in every ::Insert* invocation.
There was a problem hiding this comment.
I like *AndReturn, it's nice and explicit.
There was a problem hiding this comment.
InsertAndHandleDuplication?
There was a problem hiding this comment.
We had an offline chat and ended up agreeing on InsertKey()
|
@maysamyabandeh has updated the pull request. View: changes, changes since last import |
There was a problem hiding this comment.
@maysamyabandeh is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@maysamyabandeh is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
The MemTableRep API was broken by this commit: 813719e
This patch reverts the changes and instead adds InsertKey (and etc.) overloads to extend the MemTableRep API without breaking the existing classes that inherit from it.