Enable new KeyValueCursorBaseContinuation serialization #3671
Conversation
alecgrieser
left a comment
alecgrieser
left a comment
There was a problem hiding this comment.
Mostly looks good to me. I think I agree with the general approach here for the yaml tests, which is that we'll remove the various exclusions referencing #3206 in a subsequent minor version. Given that the new continuation logic went into 4.6.4.0, it would be nice to update the minor version to 4.8 as this feature is compatible with all 4.7 versions (but not all 4.6 or older versions). I believe @g31pranjal had a PR that was also going to target 4.8 (#3659). So, some amount of coordination is needed, either by having a third PR bump the version and merging both after this, or having one PR or the other make the minor version bump
...re/src/main/java/com/apple/foundationdb/record/provider/foundationdb/KeyValueCursorBase.java
Show resolved
Hide resolved
| synchronized (this) { | ||
| if (bytes == null) { | ||
| bytes = KeyValueCursorBase.Continuation.getInnerContinuation(new KeyValueCursorBase.Continuation(KeyValueCursorBase.Continuation.getInnerContinuation(baseContinuation.toBytes()), prefixLength, serializationMode).toBytes()); | ||
| bytes = new KeyValueCursorBase.Continuation(KeyValueCursorBase.Continuation.getInnerContinuation(baseContinuation.toBytes()), prefixLength, serializationMode).toBytes(); |
There was a problem hiding this comment.
I think this is a mistake I made in the last PR. When we are in TO_OLD, it returns the same result as the modified version.
There was a problem hiding this comment.
Okay, that makes sense. And walking through this, this only affected producing new continuations, not reading new continuations, so we should still be good to update the behavior to TO_NEW in this PR
...nal-core/src/test/java/com/apple/foundationdb/relational/recordlayer/query/ExplainTests.java
Show resolved
Hide resolved
...st/java/com/apple/foundationdb/relational/recordlayer/query/ForceContinuationQueryTests.java
Show resolved
Hide resolved
alecgrieser
left a comment
alecgrieser
left a comment
There was a problem hiding this comment.
LGTM. It would still be good to coordinate on getting this into 4.8, but the substantive changes look good
| synchronized (this) { | ||
| if (bytes == null) { | ||
| bytes = KeyValueCursorBase.Continuation.getInnerContinuation(new KeyValueCursorBase.Continuation(KeyValueCursorBase.Continuation.getInnerContinuation(baseContinuation.toBytes()), prefixLength, serializationMode).toBytes()); | ||
| bytes = new KeyValueCursorBase.Continuation(KeyValueCursorBase.Continuation.getInnerContinuation(baseContinuation.toBytes()), prefixLength, serializationMode).toBytes(); |
There was a problem hiding this comment.
Okay, that makes sense. And walking through this, this only affected producing new continuations, not reading new continuations, so we should still be good to update the behavior to TO_NEW in this PR
3 participants
No description provided.