8000 Adding optional eagerClose parameter to merge methods in FlowOps and javadsl method overloads by Athelionas · Pull Request #18959 · akka/akka · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Adding optional eagerClose parameter to merge methods in FlowOps and javadsl method overloads #18959

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

Adding optional eagerClose parameter to merge methods in FlowOps and javadsl method overloads #18959

wants to merge 4 commits into from

Conversation

Athelionas
Copy link
Contributor

@akka-ci
Copy link
akka-ci commented Nov 18, 2015

Can one of the repo owners verify this patch?

@drewhk
Copy link
Contributor
drewhk commented Nov 18, 2015

OK TO TEST

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Nov 18, 2015
@Athelionas
Copy link
Contributor Author

It looks like the build has failed due to #18961 having run after this build

@ktoso
Copy link
Contributor
ktoso commented Nov 20, 2015

PLS BUILD

@ktoso
Copy link
Contributor
ktoso commented Nov 20, 2015

LGTM, thanks @Athelionas!

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Nov 20, 2015
viktorklang
viktorklang reviewed Nov 23, 2015
View reviewed changes
akka-stream/src/main/scala/akka/stream/javadsl/Flow.scala
@@ -929,7 +929,22 @@ final class Flow[-In, +Out, +Mat](delegate: scaladsl.Flow[In, Out, Mat]) extends
* '''Cancels when''' downstream cancels
*/
def merge[T >: Out](that: Graph[SourceShape[T], _]): javadsl.Flow[In, T, Mat] =
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't document that it uses false eager close.

Copy link
Contributor

Choose a reason for hiding this comment

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

well, it does: the «completes when» clause clearly describes the behavior, that’s good enough for me

@viktorklang
Copy link
Contributor

Hi!

I commented that there was a bit of missing documentation,

Since streams do not close, but complete, I'd propose that it is eagerComplete rather than eagerClose. What do you think of that?

@Athelionas
Copy link
Contributor Author

Hi!

Thanks for the review, I'll add missing pieces to the documentation.

I named the parameter after the eagerClose param of Merge and MergePreferred, but I agree. IMHO, it should have been called eagerComplete there too. If we decide to refactor the parameter names then we should do it consistently everywhere.

@viktorklang
Copy link
Contributor

@Athelionas I completely agree. And if we rename them on Merge and MergePreferred we should add a section to the migration guides (for those who might have used their current name in code)

@ktoso
Copy link
Contributor
8000
ktoso commented Nov 23, 2015

In theory we could use @deprecatedName on the params to ease migration, not its worth it during Milestone phase though

@drewhk
Copy link
Contributor
drewhk commented Nov 23, 2015

+1 for rename

@Athelionas
Copy link
Contributor Author

I checked the documentation and I don't really know what should be added.

For example this line makes it quite clear what eagerClose (or eagerComplete) is:
'''Completes when''' all upstreams complete (eagerClose=false) or one upstream completes (eagerClose=true)

Default values should be stated though.

In case of javadsl merge method without eagerClose parameter, '''Completes when''' all upstreams complete explains the behaviour quite well. Notes about parameters passed to underlying methods seem unnecessary implementation details to me.

What's your opinion?

@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Nov 23, 2015
@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR is currently being validated by Jenkins labels Nov 23, 2015
@akka-ci akka-ci added the tested PR that was successfully built and tested by Jenkins label Nov 23, 2015
@drewhk
Copy link
Contributor
drewhk commented Nov 23, 2015

@Athelionas There were two changes discussed:

  • use eagerComplete instead of eagerClose in all stream stages, not just this use. (TCP is the only exception where close is, well, close).
  • document this name change in the migration guide (akka\akka-docs-dev\rst\scala\migration-guide-1.0-2.x-scala.rst and akka\akka-docs-dev\rst\java\migration-guide-1.0-2.x-java.rst)

@Athelionas
Copy link
Contributor Author

First point is done hopefully, second is in progress :)

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Nov 25, 2015
@ktoso
Copy link
Contributor
ktoso commented Dec 3, 2015

Hi @Athelionas,
just a friendly ping if you're still working on the remaining comments to this PR? :-)
It's a nice improvement and we'd like to pull it in :-)

Let us know if you're stuck with some part of it.

rkuhn
rkuhn reviewed Dec 11, 2015
View reviewed changes
akka-docs-dev/rst/java/migration-guide-1.0-2.x-java.rst
@@ -8,7 +8,7 @@ The 2.0 release contains some structural changes that require some
simple, mechanical source-level changes in client code.


Introduced proper named constructor methods insted of ``wrap()``
Introduced proper named constructor methods instead of ``wrap()``
================================================================
Copy link
Contributor

Choose a reason for hiding this comment

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

please fix heading marker as well (one = is missing now, sphinx is quite picky … )

@rkuhn
Copy link
Contributor
rkuhn commented Dec 11, 2015

@Athelionas please let us know if you intend to pick this up again, otherwise we’ll take it over on Monday. In any case thanks for the contribution!

@akka-ci
Copy link
akka-ci commented Dec 15, 2015

Can one of the repo owners verify this patch?

@ktoso
Copy link
Contributor
ktoso commented Dec 15, 2015

OK TO TEST

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed tested PR that was successfully built and tested by Jenkins validating PR is currently being validated by Jenkins labels Dec 15, 2015
@rkuhn
Copy link
Contributor
rkuhn commented Jan 11, 2016

Taking over.

@rkuhn rkuhn mentioned this pull request Jan 11, 2016
Merged
@rkuhn rkuhn closed this Jan 11, 2016
62D1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
needs-attention Indicates a PR validation failure (set by CI infrastructure)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Athelionas @akka-ci @drewhk @ktoso @viktorklang @rkuhn
0