Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BEAM-5974] Fix ByteKeyRangeTracker to handle tryClaim(ByteKey.EMPTY) instead of exposing markDone #6949

Merged
merged 2 commits into from Nov 7, 2018

Conversation

Projects
None yet
3 participants
@lukecwik
Copy link
Member

commented Nov 5, 2018

Add support for tryClaim(ByteKey.EMPTY) and fix doneness checking and also returning checkpoints
for restrictions that are unstarted or done.


Follow this checklist to help us incorporate your contribution quickly and easily:

  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

It will help us expedite review of your Pull Request if you tag someone (e.g. @username) to look at it.

Post-Commit Tests Status (on master branch)

Lang SDK Apex Dataflow Flink Gearpump Samza Spark
Go Build Status --- --- --- --- --- ---
Java Build Status Build Status Build Status Build Status Build Status Build Status Build Status Build Status
Python Build Status --- Build Status
Build Status
Build Status --- --- ---

@lukecwik lukecwik requested review from iemejia and swegner Nov 5, 2018

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 5, 2018

[BEAM-5974] Fix ByteKeyRangeTracker to handle tryClaim(ByteKey.EMPTY)…
… instead of exposing markDone

Add support for tryClaim(ByteKey.EMPTY) and fix doneness checking and also returning checkpoints
for restrictions that are unstarted or done.

@lukecwik lukecwik force-pushed the lukecwik:beam5974 branch from 7e5f2fb to c139f19 Nov 5, 2018

@iemejia

This comment has been minimized.

Copy link
Member

commented Nov 6, 2018

Run Java PreCommit

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2018

Run Java Precommit

@swegner

swegner approved these changes Nov 7, 2018

ByteKeyRange rval = ByteKeyRange.of(range.getStartKey(), range.getEndKey());
// We update our current range to an interval that contains no elements.
range = NO_KEYS;
return rval;

This comment has been minimized.

Copy link
@swegner

swegner Nov 7, 2018

Contributor

Newbie question: would it be incorrect to return the original range instead of creating a new one?

This comment has been minimized.

Copy link
@lukecwik

lukecwik Nov 7, 2018

Author Member

Yes, for some reason I goofed and forgot how to do a swap using a temp variable. I'll submit that as a follow up change since it is a minor optimization over the current implementation and the tests passed.

"Trying to claim key %s while last attempted key was %s",
key,
lastAttemptedKey);
lastAttemptedKey = key;

This comment has been minimized.

Copy link
@swegner

swegner Nov 7, 2018

Contributor

The javadoc @return comment states that returning false indicates a no-op; does that mean we shouldn't mutate lastAttemptedKey here?

This comment has been minimized.

Copy link
@lukecwik

lukecwik Nov 7, 2018

Author Member

The range is always a semi-bounded open interval and hence "" being the end key can never be claimed.
So updating lastAttemptedKey and returning false is the correct thing to do.

@@ -64,23 +87,36 @@ public synchronized ByteKeyRange checkpoint() {
/**
* Attempts to claim the given key.
*
* <p>Must be larger than the last successfully claimed key.
* <p>Must be larger than the last attempted key. Note that passing in {@link ByteKey#EMPTY}

This comment has been minimized.

Copy link
@swegner

swegner Nov 7, 2018

Contributor

Why do we care about whether the key is greater than last attempted rather than last claimed?

Would you mind adding a short javadoc defining what lastAttemptedKey represents? I believe it represents the last attempted claim, regardless of whether it succeeded or not.

This comment has been minimized.

Copy link
@lukecwik

lukecwik Nov 7, 2018

Author Member

Yes, will add the javadoc in the follow-up PR since tests passed.

@lukecwik lukecwik merged commit ef93dfd into apache:master Nov 7, 2018

5 checks passed

Java ("Run Java PreCommit") SUCCESS
Details
JavaPortabilityApi ("Run JavaPortabilityApi PreCommit") SUCCESS
Details
Java_Examples_Dataflow ("Run Java_Examples_Dataflow PreCommit") SUCCESS
Details
RAT ("Run RAT PreCommit") SUCCESS
Details
Spotless ("Run Spotless PreCommit") SUCCESS
Details
@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2018

Opened up #6971 to address the comments on this PR.

@iemejia

This comment has been minimized.

Copy link
Member

commented Nov 7, 2018

Sorry a bit late for this review, Is there any consequence on this and the previous change in the logic of the other ByteKeyRangeTracker (org.apache.beam.sdk.io.range.ByteKeyRangeTracker) ?

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 7, 2018

I looked through the org.apache.beam.sdk.io.range.ByteKeyRangeTracker and don't believe it needs to change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.