Skip to content

Conversation

@kennknowles
Copy link
Member

@kennknowles kennknowles commented Feb 4, 2018

This ports the Beam codebase from Guava futures to Java 8 futures. This has been discussed at length on the Beam dev list.

The standards that I have held:

  • Consumers of futures use CompletionStage. They can do async chaining and can use MoreFutures.get(...) to get the result without passing through types they shouldn't be touching.
  • Producers of futures use CompletableFuture and return it as a CompletionStage.
  • When you need to handle exceptions in a reasonable way, use MoreFutures helpers that will complete things exceptionally based on the underlying function throwing.

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

  • Make sure there is a JIRA issue filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue.
  • Write a pull request description that is detailed enough to understand:
    • What the pull request does
    • Why it does it
    • How it does it
    • Why this approach
  • Each commit in the pull request should have a meaningful subject line and body.
  • Run mvn clean verify to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
  • If this contribution is large, please file an Apache Individual Contributor License Agreement.

@kennknowles kennknowles force-pushed the futures branch 2 times, most recently from f4a34cc to 6951fb4 Compare February 5, 2018 04:16
@kennknowles
Copy link
Member Author

So many mocks ;_;

@kennknowles kennknowles force-pushed the futures branch 3 times, most recently from 28fe929 to ebe0836 Compare February 5, 2018 15:38
@kennknowles kennknowles requested review from jkff and tgroh February 5, 2018 15:39
@kennknowles
Copy link
Member Author

Requested reviews from just a couple folks in the right arenas rather than targeting the authors of every touched piece of code.

Copy link
Contributor

@jkff jkff left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM except for the easily fixed findbugs error.

ListenableFuture<DataflowPackage> stagedPackage =
Futures.transform(
stagingResult,
CompletionStage<StagingResult> stagingResult =
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the TODO above?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

*/
public class MoreFutures {

/** Like {@link Runnable} but allowed to throw any exception. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Clarify: are these interfaces intended to be used only in futures-related code? (if they are intended to be used more widely, they should be outside MoreFutures; if not, ideally that should be documented, cause it's tempting)

Copy link
Member

Choose a reason for hiding this comment

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

In fact, these exist within the SDK harness, among other places, already, and I don't believe all of those uses interact directly with futures; I'd rather just have them in util, or function, (or any other meaningful name)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

@tgroh tgroh left a comment

Choose a reason for hiding this comment

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

Same-same comment

*/
public class MoreFutures {

/** Like {@link Runnable} but allowed to throw any exception. */
Copy link
Member

Choose a reason for hiding this comment

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

In fact, these exist within the SDK harness, among other places, already, and I don't believe all of those uses interact directly with futures; I'd rather just have them in util, or function, (or any other meaningful name)

@kennknowles kennknowles force-pushed the futures branch 3 times, most recently from b6a0b80 to f890179 Compare February 5, 2018 20:04
@kennknowles
Copy link
Member Author

run java gradle precommit

@kennknowles kennknowles merged commit 8edc18e into apache:master Feb 5, 2018
@kennknowles kennknowles deleted the futures branch July 3, 2018 21:56
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.

3 participants