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-2939] SplittableDoFn Java SDK API Changes #6969

Merged
merged 7 commits into from Nov 30, 2018

Conversation

Projects
None yet
3 participants
@lukecwik
Copy link
Member

commented Nov 7, 2018


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 force-pushed the lukecwik:splittabledofn4 branch 4 times, most recently from d83c78f to b5af1a3 Nov 7, 2018

@lukecwik lukecwik force-pushed the lukecwik:splittabledofn4 branch 2 times, most recently from f6916c6 to aa02a6e Nov 14, 2018

@lukecwik lukecwik changed the title [DISCUSS] SplittableDoFn Java SDK API Changes [BEAM-2939] SplittableDoFn Java SDK API Changes Nov 14, 2018

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

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

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

Run Java PreCommit

1 similar comment
@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

Run Java PreCommit

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2018

This has been out for a while, limited feedback on the mailing list so was hoping to proceed with getting this in so I can work on reporting backlog and splitting within sdks/java/harness.

@iemejia
Copy link
Member

left a comment

Still one test complaining. let some minor comments. I will move my 'understanding' questions into the ML. But LGTM up to where I understand.

package org.apache.beam.sdk.transforms.splittabledofn;

/** Definitions and convenience methods for working with restrictions. */
public final class Restrictions {

This comment has been minimized.

Copy link
@iemejia

iemejia Nov 15, 2018

Member

This seems not to be used yet (also I don't understand fully its behavior), maybe add later.

This comment has been minimized.

Copy link
@lukecwik

lukecwik Nov 20, 2018

Author Member

I replied to this in the mailing list discuss thread.

* defining the method and returning {@code Collections.singletonList(restriction)}).
* defining the method and outputting {@code Collections.singletonList(restriction)}).
*
* <p>TODO: Make the InputT parameter optional.

This comment has been minimized.

Copy link
@iemejia

iemejia Nov 15, 2018

Member

This refers to the implementation in the invoker part, is it? Maybe worth doing it as part of this.

This comment has been minimized.

Copy link
@lukecwik

lukecwik Nov 20, 2018

Author Member

Yes, this refers to the implementation for the invoker part.

I was hoping that I could get the API part done without having the generic invoker logic done since that allows parallelizing work amongst other contributors. Will file JIRA about adding support for making these optional and moving towards a generic invoker / method format once this gets ship it.

API changes for SplittableDoFn to support reporting backlog, splittin…
…g with backlog and also adding support for bundle finalization.

@lukecwik lukecwik force-pushed the lukecwik:splittabledofn4 branch from eb3c764 to 60a9adb Nov 28, 2018

Address PR and mailing list comments.
Expand on the definition of Backlog partition identifier and also migrate to use a decimal representation for backlog in the proto and BigDecimal within the Java SDK.

@lukecwik lukecwik force-pushed the lukecwik:splittabledofn4 branch from 716b7ed to 8579dfa Nov 28, 2018

@swegner
Copy link
Contributor

left a comment

LGTM. I left a few minor doc comments. I'll let @iemejia finish his review before merging.

@@ -186,6 +186,15 @@ message ProcessBundleDescriptor {
org.apache.beam.model.pipeline.v1.ApiServiceDescriptor state_api_service_descriptor = 7;
}


// Represents a non-negative decimal number: unscaled_value * 10^(-scale)

This comment has been minimized.

Copy link
@swegner

swegner Nov 28, 2018

Contributor

This is well-known as 'scientific notation' (or alternatively from wikipedia: scientific form, standard index form, or standard form in the UK). I recommend noting the existing terminology here so the reader recognizes it.

This comment has been minimized.

Copy link
@lukecwik

lukecwik Nov 28, 2018

Author Member

done

@@ -186,6 +186,15 @@ message ProcessBundleDescriptor {
org.apache.beam.model.pipeline.v1.ApiServiceDescriptor state_api_service_descriptor = 7;
}


// Represents a non-negative decimal number: unscaled_value * 10^(-scale)
message Decimal {

This comment has been minimized.

Copy link
@swegner

swegner Nov 28, 2018

Contributor

Is it worth noting the unlimited precision in the message name? I've seen libraries use names like BigDecimal, LargeDecimal, etc.

This comment has been minimized.

Copy link
@lukecwik

lukecwik Nov 28, 2018

Author Member

I have seen other languages just call this decimal so I would prefer to refer to it as that.

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

Run Java PreCommit

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

Spoke with Ismael over Slack and he said to go ahead with the merge.

lukecwik added some commits Nov 29, 2018

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

Run Java PreCommit

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

Run Portable_Python PreCommit

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

Failing Run Portable_Python PreCommit is for a non existent task and WIP PR #7157

@lukecwik

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2018

Run Java PreCommit

@lukecwik lukecwik merged commit d136637 into apache:master Nov 30, 2018

8 checks passed

Go ("Run Go PreCommit") SUCCESS
Details
Java ("Run Java PreCommit") SUCCESS
Details
JavaPortabilityApi ("Run JavaPortabilityApi PreCommit") SUCCESS
Details
Java_Examples_Dataflow ("Run Java_Examples_Dataflow PreCommit") SUCCESS
Details
Portable_Python ("Run Portable_Python PreCommit") SUCCESS
Details
Python ("Run Python PreCommit") SUCCESS
Details
RAT ("Run RAT PreCommit") SUCCESS
Details
Spotless ("Run Spotless PreCommit") SUCCESS
Details

kennknowles added a commit to kennknowles/beam that referenced this pull request Jan 23, 2019

Revert "[BEAM-2939] SplittableDoFn Java SDK API Changes (apache#6969)"
This reverts commit d136637.

The commit caused SplittableDoFnTest#testLateData to hang indefinitely.

Minor conflicts during revert hand-resolved.

swegner added a commit that referenced this pull request Jan 23, 2019

swegner added a commit to swegner/beam that referenced this pull request Jan 23, 2019

@swegner swegner referenced this pull request Jan 23, 2019

Merged

Cherry-pick PR #7600 to 2.10 release #7603

0 of 2 tasks complete
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.