Resolves #1663: Add .map variant to RecordCursor that supports changing a result's continuation #1664
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for Linux CentOS 7
|
ScottDugas
requested changes
May 11, 2022
| // Stripping away the prefix and resuming the cursor should produce the rest of the list | ||
| byte[] continuation = result.getContinuation().toBytes(); | ||
| assertNotNull(continuation); | ||
| RecordCursor<Integer> tailCursor = RecordCursor.fromList(ints, Arrays.copyOfRange(continuation, prefix.size(), continuation.length)); |
There was a problem hiding this comment.
If only to provide an example, it would probably be good to have this also map the continuation.
There was a problem hiding this comment.
I do wonder whether this is a problematic api, since:
RecordCursor.fromList(ints, continuation).mapContinuation(continuation -> { ... });
could be surprising, since the continuation does not line up.
This is similar to flatMapPipelined, and perhaps this should be a static method like that, but doing so would require providing a ContinuationConverter class (or two lambdas) that can do bidirectional conversion.
Potentially more importantly, the code as you have it might make it harder to decouple the code creating the continuation from the one mapping the continuation. Maybe, it's not a real issue, I'm having trouble thinking of a situation where there isn't something that passes the continuation into the code creating cursor, and also calls mapContinuation.
There was a problem hiding this comment.
Yeah, the way that it's the caller's responsibility to manipulate the continuation does feel like it could be a bit of a foot-gun. The alternative using a static method might look something like this:
interface ContinuationConvertor {
@Nullable
byte[] unwrapContinuation(@Nullable byte[] continuation);
RecordCursorContinuation wrapContinuation(@Nonnull RecordCursorContinuation continuation);
}
static <T> RecordCursor<T> mapContinuation(@Nonnull Function<byte[], RecordCursor<T>> cursorFunction, @Nonnull ContinuationConvertor convertor, @Nullable byte[] continuation) {
byte[] innerContinuation = convertor.unwrapContinuation(continuation);
return cursorFunction.apply(innerContinuation)
.mapContinuation(convertor::wrapContinuation);
}(Note that it uses the current .mapContinuation value in its implementation, but that could theoretically be rewritten.)
Which...maybe isn't too bad, but it doesn't feel great.
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for Linux CentOS 7
|
ScottDugas
requested changes
May 16, 2022
| } | ||
|
|
||
| /** | ||
| * Get a new cursor by applying transforming the continuation of each result. This function creates |
There was a problem hiding this comment.
Suggested change
| * Get a new cursor by applying transforming the continuation of each result. This function creates | |
| * Get a new cursor by applying a transformation to the continuation of each result. This function creates |
alecgrieser
force-pushed
the
cursor-map-continuations
branch
from
3ae777b
to
1a74148
Compare
|
Rebase addresses merge conflict |
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for Linux CentOS 7
|
…ports changing a result's continuation This adds a variant to `RecordCursor.map` that allows the caller to adjust the continuations returned by a `RecordCursor`. This would allow, for example, for someone to wrap the continuations with new data, or otherwise adjust the continuations returned. This resolves FoundationDB#1663.
… the user think about both wrapping and unwrapping the continuation
alecgrieser
force-pushed
the
cursor-map-continuations
branch
from
1a74148
to
844ee07
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
alecgrieser
force-pushed
the
cursor-map-continuations
branch
from
844ee07
to
08b1be3
Compare
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for Linux CentOS 7
|
AWS CodeBuild CI Report for Linux CentOS 7
|
ScottDugas
previously approved these changes
May 17, 2022
alecgrieser
force-pushed
the
cursor-map-continuations
branch
from
08b1be3
to
d04a5a5
Compare
|
Made a trivial commit (changing commit date) to cause PRBs to re-run |
AWS CodeBuild CI Report for Linux CentOS 7
|
|
Kudos, SonarCloud Quality Gate passed! |
AWS CodeBuild CI Report for Linux CentOS 7
|
ScottDugas
approved these changes
May 19, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a variant to
RecordCursor.mapthat allows the caller to adjust the continuations returned by aRecordCursor. This would allow, for example, for someone to wrap the continuations with new data, or otherwise adjust the continuations returned.This resolves #1663.