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.

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

alsoTo eager cancellation #24291 #24423

Merged
merged 1 commit into from
Feb 19, 2018
Merged

alsoTo eager cancellation #24291 #24423

merged 1 commit into from
Feb 19, 2018

Conversation

jeremystone
Copy link
Contributor

No description provided.

@akka-ci
Copy link

akka-ci commented Jan 26, 2018

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

@johanandren
Copy link
Member

OK TO TEST

johanandren
johanandren approved these changes Jan 26, 2018
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM

akka-docs/src/main/paradox/stream/stages-overview.md Outdated
@@ -716,6 +716,8 @@ Attaches the given `Sink` to this `Flow`, meaning that elements that pass throug

**completes** when upstream completes

**Cancels** when downstream or `Sink` cancels
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: the other entries ones start with a lowercase letter

@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Jan 26, 2018
@raboof
Copy link
Member

raboof commented Jan 26, 2018

Refs #24291

@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jan 26, 2018
@akka-ci
Copy link

akka-ci commented Jan 26, 2018

Test FAILed.

@johanandren
Copy link
Member

Seems there are a few tests relying on the old behavior of alsoTo.

@jeremystone
Copy link
Contributor Author

@johanandren. Indeed.

The tests make use of alsoTo to force a behaviour that could be achieved by other means (at least by a raw Broadcast). But who else relies on this?

Should I fix up the tests or do we (you guys) need to rethink and maybe have eagerCancel as a parameter of alsoTo?

@johanandren
Copy link
Member

I still think eager cancellation is the intuitive behavior, and maybe the other case, where you don't care so much is satisfied with wireTap (not merged yet: #23824)

@jeremystone
Copy link
Contributor Author

OK. Will take a look at fixing up the tests when I get chance.

patriknw
patriknw approved these changes Jan 29, 2018
View reviewed changes
Copy link
Member

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

LGTM

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

Please can someone check I haven't altered the semantics of the three tests I fixed that were affected by the changed behaviour of alsoTo. (Once wireTap (#23824) is merged in, these can probably be reverted to using that instead?)

@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Jan 29, 2018
@akka-ci
Copy link

akka-ci commented Jan 29, 2018

Test FAILed.

@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 Jan 29, 2018
@akka-ci
Copy link

akka-ci commented Jan 29, 2018

Test PASSed.

johanandren
johanandren approved these changes Jan 30, 2018
View reviewed changes
Copy link
Member

@johanandren johanandren left a comment

Choose a reason for hiding this comment

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

LGTM

@johanandren
Copy link
Member

Sorry about us being slow here, I think this is ready for merge, but now needs a rebase first.

@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 Feb 18, 2018
@akka-ci
Copy link

akka-ci commented Feb 18, 2018

Test PASSed.

@johanandren johanandren merged commit e766207 into akka:master Feb 19, 2018
@johanandren
Copy link
Member

Thanks!

@jeremystone jeremystone deleted the wip-alsoTo-eagerCancel branch February 19, 2018 08:48
@jeremystone
Copy link
Contributor Author

No problem. Glad I could help.

ktoso
ktoso reviewed Mar 5, 2018
View reviewed changes
akka-stream/src/main/scala/akka/stream/javadsl/SubSource.scala
@@ -1137,7 +1137,7 @@ class SubSource[+Out, +Mat](delegate: scaladsl.SubFlow[Out, Mat, scaladsl.Source
*
* '''Completes when''' upstream completes
*
* '''Cancels when''' downstream cancels
* '''Cancels when''' downstream or Sink cancels
*/
def alsoTo(that: Graph[SinkShape[Out], _]): SubSource[Out, Mat] =
Copy link
Member

Choose a reason for hiding this comment

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

Could we have introduced an overload with the eager parameter?

Copy link
Member

Choose a reason for hiding this comment

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

I think it was discussed and if you don't care about the side stream you should use the new wireTap (or graph dsl with Broadcast for power user full control)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that's a good idea; that conflates the 2 decisions:

  • if the "other" should backpressure the upstream (wireTap -- things get dropped for "other"; alsoTo -- things are backpressured)
  • and what happens when "other" cancels (wireTap -- "other" completing does not cancel upstream; alsoTo -- completion of "other" does cancel the upstream)

But dropping on backpressure into "other" yet cancelling if it terminates could also be a valid use case?

@WellingR WellingR mentioned this pull request Sep 3, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso left review comments

@patriknw patriknw patriknw approved these changes

@johanandren johanandren johanandren approved these changes

Assignees
No one assigned
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jeremystone @akka-ci @johanandren @raboof @ktoso @patriknw