Unify SET_VERSIONSTAMPED_KEY and SET_VERSIONSTAMPED_VALUE API #242
Conversation
fdbclient/NativeAPI.actor.cpp
Outdated
| auto r = singleKeyRange( key, req.arena ); | ||
| auto v = ValueRef( req.arena, operand ); | ||
| auto r = singleKeyRange( key, req.arena ); | ||
| auto v = ValueRef( req.arena, operand ); |
There was a problem hiding this comment.
Curses! Foiled again!
| } else if (m.type == MutationRef::SetVersionstampedValuePos ) { | ||
| transformSetVersionstampedValuePos( m, requests[0].version, transactionNumberInBatch ); | ||
| } else if (m.type == MutationRef::SetVersionstampedValue) { | ||
| transformVersionstampMutation( m, &MutationRef::param2, requests[0].version, transactionNumberInBatch ); |
There was a problem hiding this comment.
git clang-format might be easier to do
There was a problem hiding this comment.
So, our .clang-format is currently checked in only on the master branch. Does that need to be fixed?
There was a problem hiding this comment.
This particular alignment change I had fixed in a later commit, but it wouldn't surprise me if there are some other formatting things sitting elsewhere.
flow/Arena.h
Outdated
| Standalone<StringRef> withPrefix( const StringRef& prefix ) const { | ||
| Standalone<StringRef> r; | ||
| ((StringRef &)r) = withPrefix(prefix, r.arena()); | ||
| return r; | ||
| } | ||
|
|
||
| Standalone<StringRef> withSuffix( const StringRef& suffix ) const { | ||
| Standalone<StringRef> r; | ||
| ((StringRef &)r) = withSuffix(suffix, r.arena()); |
There was a problem hiding this comment.
r.contents() = is a less questionable way to do this.
There was a problem hiding this comment.
Fixed. I was following the same convention as withPrefix, so I went and changed both of them.
| * @deprecated Since 5.2.0, replaced with {@link #packVersionstampedKey(Tuple) packVersionstampedKey()} | ||
| */ | ||
| @Deprecated | ||
| public byte[] packWithVersionstamp(Tuple tuple) { |
There was a problem hiding this comment.
Oh, and do these removals not need to be api versioned somehow? or are they already?
There was a problem hiding this comment.
Making packWithVersionstamp deprecated was added within this PR when it looked like there would be different semantics for specifying the version offset with a key and with a value. This removal just gets us to status quo ante pull request.
…es to an arbitrary place in the value
…support in bindings and bindingtester
… on API version to make them consistent
alecgrieser
force-pushed
the
32437306-better-versionstamped-value
branch
from
62df5ab
to
8de914a
Compare
|
I believe Alex said somewhere that I should merge this after I addressed the comments, so that's what I'm going to do. |
This modifies both the SET_VERSIONSTAMP_KEY and SET_VERSIONSTAMPED_VALUE atomic mutations so that they do the same thing. In particular, they now both take four bytes of position at the end of the key/value and use that as an offset in which to place the versionstamp.
This uses the API version to allow older code to continue to use 2 bytes for keys and 0 bytes for values. The bindingtester is updated to write atomic mutations that are in the correct form, and the tuple packing methods will do the right thing, so old code that used those abstractions doesn't need to change (for keys). IMO, the most likely place for error is that I have updated some place to always use four bytes when it needs to look at the API version before deciding whether it should use the old format or the new format.
This modifies the API so is somewhat 🌶 by its nature. I am somewhat hesitant to submit it, but the alternative is either not letting the value set a position for its versionstamp, which is pretty annoying, or having the "key"-based API take two bytes while the "value"-based API takes four bytes. (It needs more than two bytes because the max value size is 100k, which astute observers will note requires more than 16 bits to represent, and four bytes is easier than three bytes to deal with.)
This addresses issue #148.