Support Supervision.restart for SubFlow's

[mdedetrich]

Provide support for restarting subflow.

[He-Pin]

the cause can be NoMoreElementsNeeded | StageWasCompleted should test that first.

[mdedetrich]

Isn't this being tested in a different way later down with isClosed/hasBeenPulled? Also I had a look at parts of the Pekko codebase that implements Supervision.restart and I don't see NoMoreElementsNeeded | StageWasCompleted being tested in this way.

This doesn't mean its wrong, but if thats the case then testing NoMoreElementsNeeded | StageWasCompleted would be unique to SubFlow and I am not sure why?

[He-Pin]

NoMoreElementsNeeded | StageWasCompleted indicate the downstream cancel without exception, then that should not go through the supervision decider ? @raboof @jrudolph may have better understanding than me , ping~

[mdedetrich]

@He-Pin @jxnu-liguobin @Roiocam @raboof

So I just noticed that for SubFlow's in general the supervision strategy doesn't properly propagate (see comment at https://github.com/apache/incubator-pekko/blob/c44c0b7cbdab11d85176cfe062288fdcba16c56a/stream-tests/src/test/scala/org/apache/pekko/stream/scaladsl/FlowSplitAfterSpec.scala#L213) which mean that this PR isn't going to work until we solve the fundamental underlying issue.

Due to this I will set the PR to blocked and make another issue on this point

[pjfanning]

I'm planning to cut an RC for 1.1.0-M1. This will likely not be in it - but can be included in 1.1.0-M2.

[mdedetrich]

I'm planning to cut an RC for 1.1.0-M1. This will likely not be in it - but can be included in 1.1.0-M2.

There is an actual bug with the functionality introduced in #252, i.e. Supervision.resumeStrategy doesn't actually resume when an exception is thrown.

This needs to be solved for 1.1.0, ideally in -M1 but I don't know when you wanted to make the actual release (I guess after incubation is fully passed?)

[mdedetrich]

Bug made at #1205

[pjfanning]

is there a chance that we could revert #252 and try a full change for 1.1.0-M2?

[mdedetrich]

is there a chance that we could revert #252 and try a full change for 1.1.0-M2?

We are actually using this feature in production so I would try and avoid that. If fixing the bug ends up taking too long I would propose we fix it in 1.1.0-M2 since no one right now is actually relying on Supervision.resumeStrategy catching and resuming exceptions

[pjfanning]

I'm only to delaying the 1.1.0-M1-RC1 until next week if this a reasonable chance that this can be addressed soon.

[mdedetrich]

I'm only to delaying the 1.1.0-M1-RC1 until next week if this a reasonable chance that this can be addressed soon.

Agreed

[mdedetrich]

So I had a look at this after it was unblocked and it does seem to be harder than normal, this is because a new SubstreamHandler is created from the original input port which makes things complicated if we have already handed over to the new SubStreamHandler because we have crossed the boundary of the split decision. I think to solve this properly we need to persist already retrieved elements.

Furthermore its not just Split we have to support but also Group (the other operator that creates SubFlow). For this reason and in the interest of also reducing the number of milestones ill drop this unless I happen to be missing something.

@pjfanning @He-Pin @Roiocam wdyt?