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-7157] Allow creation of BinaryCombineFn from lambdas. #8409

Merged
merged 2 commits into from Apr 30, 2019

Conversation

robertwb
Copy link
Contributor

This came up in a recent user thread.


Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

  • Choose reviewer(s) and mention them in a comment (R: @username).
  • 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.

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 Build Status
Python Build Status
Build Status
--- Build Status
Build Status
Build Status --- --- ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website
Non-portable Build Status Build Status Build Status Build Status
Portable --- Build Status --- ---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

@robertwb
Copy link
Contributor Author

R: @youngoli

@robertwb
Copy link
Contributor Author

Run Java PreCommit

@robertwb
Copy link
Contributor Author

I keep getting timeouts in org.apache.beam.runners.dataflow.worker.fn.BeamFnControlServiceTest.testClientConnecting which are irrelevant to this change.

@FunctionalInterface
public interface SerializableBiFunction<FirstInputT, SecondInputT, OutputT>
extends BiFunction<FirstInputT, SecondInputT, OutputT>, Serializable {
// This class is empty, but required for creating serializable lambda expressions.
Copy link
Member

@lukecwik lukecwik Apr 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// This class is empty, but required for creating serializable lambda expressions.
// This interface is empty, but required for creating serializable lambda expressions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Done.

};
}

public static BinaryCombineFn<Long> foo() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these two functions (foo and bar) supposed to be here? Looks like testing code that was left behind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes, I was validating what inference could and could not accept. It's now in the test file but I forgot to delete it here. Thanks.

@youngoli
Copy link
Contributor

Run Java PreCommit

@youngoli
Copy link
Contributor

testClientConnecting is still failing. I see some precommits are passing. I can't tell if it's flaky or if the fix is in master and isn't included in this PR. I think you should try to get the Java Precommit passing if you can, but the change itself looks good so I'm approving it.

@robertwb
Copy link
Contributor Author

Run Java PreCommit

@robertwb
Copy link
Contributor Author

Thanks for the review.

Looks like this precommit is now failing more often than not, and spot checking reveals this test to be the culprit. This change is clearly unrelated so I'm going to go ahead and merge.

@robertwb robertwb merged commit 4389dcf into apache:master Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants