Replace SubstreamCancelStrategy with SupervisionDecider for Split by mdedetrich · Pull Request #252 · apache/pekko

mdedetrich · 2023-03-18T11:37:24Z

This PR is the equivalent of akka/akka-core#31415 but for Pekko, I would recommend reading the conversation thread from that PR to get the context/justification for the feature.

The very brief summary is that the SubstreamCancelStrategy abstraction that is only used when creating a SubFlow (via usage of splitWhen and other associated functions) is not needed since Pekko already provides a generic streams abstraction for this called a SupervisionDecider. It can even be said that the SubstreamCancelStrategy is worse than pointless duplication because unlike SupervisionDecider you can't do things like log the thrown exception in the SubFlow (this is actually the main impetus for the change, because as user I had the situation where SubFlow was throwing exceptions and I had no idea what was going on because it wasn't logging them via a custom SupervisionDecider which I defined, even worse because its not possible to change the already defined SubstreamCancelStrategy you have to do annoying workarounds to log any potentially thrown exceptions in a SubFlow).

Note that this change although not source/binary breaking it does change behaviour because the default SupervisionDecider is to stop in case of an exception where as the default SubstreamCancelStrategy default is to ignore and keep on going (this is also the reason behind the test changes in the PR). As stated in the original PR, even though it is a breaking behavioural change, since it fails more explicitly its much easier for a user to notice (see akka/akka-core#31415 (review)).

Due to this behaviour change the PR was intended to be looked at by @raboof and @jrudolph to get buy in/opinion whether this change is acceptable (see akka/akka-core#31415 (comment)), would be great to get feedback on this PR's premise (because otherwise we have to create an entirely new set of functions which overall I think is worse proposition).

PR is currently draft because I didn't update the docs for the behavioural change. If @jrudolph / @raboof are happy with this change (along with anyone else that has strong experience with akka/pekko-streams, i.e. @He-Pin ) then I will update the PR with proper documentation explaining all the changes. It will also target Pekko 1.1.x.

He-Pin · 2023-03-18T11:55:52Z

I will read this PR in more detail, and add feedback later.

He-Pin · 2023-03-18T12:36:16Z

The original issue is : akka/akka-core#31394

He-Pin · 2023-03-18T16:18:02Z

Pekko-HTTP is using the splitAfter and splitWhen IIRC, how about running a snapshot against it too?

mdedetrich · 2023-03-18T17:24:14Z

Pekko-HTTP is using the splitAfter and splitWhen IIRC, how about running a snapshot against it too?

Sure I can do this, I would expect that pekko-http 1.1.x would depend on pekko 1.1.x so if there are any breaking changes in pekko-http it would just be for 1.1.x.

I can check locally what impact this change would have against pekko-http

mdedetrich · 2023-12-08T01:08:42Z

Pekko-HTTP is using the splitAfter and splitWhen IIRC, how about running a snapshot against it too?

See apache/pekko-http#371 which is related

mdedetrich · 2023-12-08T03:04:16Z

So I have decided to finish off this PR (which just involved updating the documentation) since it makes sense to release this as part of Pekko 1.1.0. Of note is that I had to add a new migration-guide-1.0.x-1.1.x.md which follows the same style Akka used (I copied it from https://github.com/mdedetrich/akka-apache/blob/main/akka-docs/src/main/paradox/project/migration-guides.md and https://github.com/mdedetrich/akka-apache/blob/main/akka-docs/src/main/paradox/project/migration-guide-2.5.x-2.6.x.md which is the latest Apache 2.0 release of Akka).

Regarding the behavioural changes, as you can see from https://github.com/mdedetrich/akka-apache/blob/main/akka-docs/src/main/paradox/project/migration-guide-2.5.x-2.6.x.md?plain=1#L17-L18, doing behavioural changes across minor updates do happen as long as there is a justification for it and I believe this PR does hit that threshold. These changes are clearly documented in migration-guide-1.0.x-1.1.x.md.

There is one other final note, the current implementation of this PR alias's the behaviour of Supervision.restart to Supervision.stop in the context of SubFlow because at least on the surface to me, Supervision.restart doesn't make any real sense for errors thrown in the resulting SubFlow's. If I am wrong here (either in that it should instead be aliased to Supervision.resume or that retrying in the context of SubFlow does make sense in which case I should implement it) then let me know.

mdedetrich · 2023-12-17T21:08:54Z

Pinging @He-Pin , @pjfanning , @jrudolph and @raboof since I forgot to do so earlier.

docs/src/main/paradox/project/migration-guide-1.0.x-1.1.x.md

docs/src/main/paradox/stream/operators/Source-or-Flow/splitAfter.md

mdedetrich · 2023-12-17T22:00:32Z

@pjfanning All apostrophe's have been removed

pjfanning

lgtm - seems sensible to not need the SubstreamCancelStrategy. I would like to see some other approvals though.

nvollmar

lgtm

He-Pin · 2023-12-18T08:38:45Z

@mdedetrich I have left some comments

mdedetrich · 2023-12-18T11:45:05Z

@mdedetrich I have left some comments

I can't see any, I think you forgot to submit?

He-Pin

I updated my comments

stream/src/main/scala/org/apache/pekko/stream/scaladsl/Flow.scala

stream/src/main/scala/org/apache/pekko/stream/impl/fusing/StreamOfStreams.scala

stream/src/main/scala/org/apache/pekko/stream/javadsl/Flow.scala

stream/src/main/scala/org/apache/pekko/stream/impl/fusing/StreamOfStreams.scala

mdedetrich · 2023-12-20T10:19:05Z

@He-Pin Responded to comments. I think that when you looked at the PR you didn't expand out the diff's and missed the fact that a lot of the code you commented on was within @InternalApi making the comments irrelevant.

He-Pin

I think this one is ok, I missed it's was internalApi.

mdedetrich · 2023-12-20T11:45:04Z

I think this one is ok, I missed it's was internalApi.

Thanks

I will leave the PR open till the end of the month (given that its christmas) to see if anyone else wants to comment on it, at that point I will merge it.

mdedetrich · 2023-12-20T23:42:19Z

The commit I just pushed was adding @deprecation in some places that I missed beforehand, otherwise PR is the same.

mdedetrich · 2023-12-28T04:46:33Z

Im going to go ahead and merge this, largely so that migration-guide-1.0.x-1.1.x.md file which this PR introduces can be used for any future possible behaviourally incompatible changes but also so I can test the changes sooner rather than later to iron out any potential problems (of particular note is pekko-http which actually uses sub flow).

mdedetrich · 2023-12-29T03:37:23Z

Just wanted to mention that since the changes in this PR were published as a snapshot, pekko-http tests ran against these changes and passed without any problems (see https://github.com/apache/incubator-pekko-http/actions/runs/7353051075/job/20018534864)

mdedetrich added enhancement New feature or request help wanted Extra attention is needed labels Mar 18, 2023

mdedetrich marked this pull request as draft March 18, 2023 11:37

mdedetrich added this to the 1.1.0 milestone Mar 18, 2023

mdedetrich force-pushed the use-supervisiondecider-with-split branch from fcf46b5 to b7ff620 Compare March 18, 2023 11:43

mdedetrich mentioned this pull request Dec 8, 2023

Make .splitWhen explicit with SubstreamCancelStrategy.drain apache/pekko-http#371

Closed

mdedetrich force-pushed the use-supervisiondecider-with-split branch from b7ff620 to 93710fd Compare December 8, 2023 03:00

mdedetrich marked this pull request as ready for review December 8, 2023 03:00

mdedetrich requested review from He-Pin, gmethvin, jrudolph, nvollmar, pjfanning and raboof December 8, 2023 03:00

mdedetrich force-pushed the use-supervisiondecider-with-split branch 3 times, most recently from 1487124 to f03c736 Compare December 8, 2023 03:59

pjfanning requested changes Dec 17, 2023

View reviewed changes

mdedetrich force-pushed the use-supervisiondecider-with-split branch from f03c736 to 32c5d2c Compare December 17, 2023 22:00

pjfanning approved these changes Dec 17, 2023

View reviewed changes

He-Pin requested a review from jxnu-liguobin December 18, 2023 04:27

nvollmar approved these changes Dec 18, 2023

View reviewed changes

He-Pin reviewed Dec 20, 2023

View reviewed changes

He-Pin approved these changes Dec 20, 2023

View reviewed changes

mdedetrich removed the help wanted Extra attention is needed label Dec 20, 2023

mdedetrich force-pushed the use-supervisiondecider-with-split branch from 32c5d2c to 2b2958b Compare December 20, 2023 22:51

Replace SubstreamCancelStrategy with SupervisionDecider for Split

5df953f

mdedetrich force-pushed the use-supervisiondecider-with-split branch from 2b2958b to 5df953f Compare December 20, 2023 23:04

mdedetrich merged commit b818925 into apache:main Dec 28, 2023

mdedetrich deleted the use-supervisiondecider-with-split branch December 28, 2023 04:46

pjfanning added the release notes Need to release note label Dec 28, 2023

mdedetrich mentioned this pull request Dec 29, 2023

Remove deprecated splitAfter functionality Aiven-Open/guardian-for-apache-kafka#521

Merged

mdedetrich mentioned this pull request Jan 19, 2024

Feature request: RestartWithBackOffAndFallback for Flow & Sink #980

Open

He-Pin mentioned this pull request Mar 18, 2024

release notes for 1.1.0-M1 #1204

Merged

This was referenced Mar 18, 2024

Support Supervision.restart for SubFlow's #981

Draft

SubFlow's don't correctly propagate Supervision.resumeStrategy #1205

Closed

Fix uncaught decider exception in Split with Supervision.resumingDecider #1207

Merged

gwak mentioned this pull request Jul 9, 2024

Pekko 1.2, Pekko-Http 1.3, Jackson 2.20 playframework/playframework#12662

Merged

7 tasks

pjfanning removed the release notes Need to release note label Aug 14, 2024

Conversation

mdedetrich commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

He-Pin commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

He-Pin commented Mar 18, 2023

Uh oh!

He-Pin commented Mar 18, 2023

Uh oh!

mdedetrich commented Mar 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdedetrich commented Dec 8, 2023

Uh oh!

mdedetrich commented Dec 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdedetrich commented Dec 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdedetrich commented Dec 17, 2023

Uh oh!

pjfanning left a comment

Choose a reason for hiding this comment

Uh oh!

nvollmar left a comment

Choose a reason for hiding this comment

Uh oh!

He-Pin commented Dec 18, 2023

Uh oh!

mdedetrich commented Dec 18, 2023

Uh oh!

He-Pin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mdedetrich commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

He-Pin left a comment

Choose a reason for hiding this comment

Uh oh!

mdedetrich commented Dec 20, 2023

Uh oh!

mdedetrich commented Dec 20, 2023

Uh oh!

mdedetrich commented Dec 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mdedetrich commented Dec 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mdedetrich commented Mar 18, 2023 •

edited

Loading

He-Pin commented Mar 18, 2023 •

edited

Loading

mdedetrich commented Mar 18, 2023 •

edited

Loading

mdedetrich commented Dec 8, 2023 •

edited

Loading

mdedetrich commented Dec 20, 2023 •

edited

Loading

mdedetrich commented Dec 28, 2023 •

edited

Loading