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.

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

Allow RestartFlow/RestartSink to restart only on failures #24421

Closed
nachinius opened this Issue Jan 25, 2018 · 9 comments

Comments

Projects
None yet
4 participants
@nachinius
Contributor

nachinius commented Jan 25, 2018

Currently it restarts when failing and when completing. Provide a way to only restart if the wrapped flow fails.

Similar to #23881 but on RestartFlow
Requested by @patriknw in #23911 (comment)

I'm working on it

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jan 26, 2018

Member

Great, thanks 👍
Perhaps the same should at the same time be added to the RestartSink so all have the same ops?

Member

ktoso commented Jan 26, 2018

Great, thanks 👍
Perhaps the same should at the same time be added to the RestartSink so all have the same ops?

@nachinius

This comment has been minimized.

Show comment
Hide comment
@nachinius

nachinius Jan 29, 2018

Contributor

@ktoso Maybe we could do it after, the RestartFlow looks good. May be in another ticket?

Contributor

nachinius commented Jan 29, 2018

@ktoso Maybe we could do it after, the RestartFlow looks good. May be in another ticket?

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jan 30, 2018

Member

Separate PR sounds good, would you want to give it a shot then as well? Reviewing the current one for now then

Member

ktoso commented Jan 30, 2018

Separate PR sounds good, would you want to give it a shot then as well? Reviewing the current one for now then

patriknw added a commit that referenced this issue Jan 30, 2018

Merge pull request #24441 from nachinius/wip-24421-add-restartflow-wi…
…th-backoff-only-on-failures

Adds RestartFlow to restart only on failures #24421

@patriknw patriknw changed the title from Allow RestartFlow.withBackoff to restart only on failures to Allow RestartFlow/RestartSink to restart only on failures Jan 30, 2018

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Jan 30, 2018

Member

@nachinius Merged the PR for RestartFlow. Keeping this ticket open for RestartSink also.

Member

patriknw commented Jan 30, 2018

@nachinius Merged the PR for RestartFlow. Keeping this ticket open for RestartSink also.

@nachinius

This comment has been minimized.

Show comment
Hide comment
@nachinius

nachinius Jan 30, 2018

Contributor

cool

Contributor

nachinius commented Jan 30, 2018

cool

@nachinius

This comment has been minimized.

Show comment
Hide comment
@nachinius

nachinius Jan 31, 2018

Contributor

Does it make sense for a RestartSink only on failures? Given that a sink can not emit failures, only cancels, there is no simple way (as far as I understand) to distinguish between a failure and a normal cancelation. Thoughts?

Contributor

nachinius commented Jan 31, 2018

Does it make sense for a RestartSink only on failures? Given that a sink can not emit failures, only cancels, there is no simple way (as far as I understand) to distinguish between a failure and a normal cancelation. Thoughts?

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jan 31, 2018

Member

Hmm, you're right, we can't act on just the reactive streams signals between stages there. I think it could still could be interesting to provide but we'd need to apply some trickery to pull it off... Technically we know the difference internally between a stage throwing an exception for example, and completing successfully, so we could use this signal for it.

Since we (akka, the graph stage interpreter) knows if a stage has failed or just completed. It may be difficult to pull off. Feel free to try it out however don't feel obliged to PR this, you're right that it's much more tricky

Member

ktoso commented Jan 31, 2018

Hmm, you're right, we can't act on just the reactive streams signals between stages there. I think it could still could be interesting to provide but we'd need to apply some trickery to pull it off... Technically we know the difference internally between a stage throwing an exception for example, and completing successfully, so we could use this signal for it.

Since we (akka, the graph stage interpreter) knows if a stage has failed or just completed. It may be difficult to pull off. Feel free to try it out however don't feel obliged to PR this, you're right that it's much more tricky

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jan 31, 2018

Member

I'll make a separate ticket about the RestartSink

Member

ktoso commented Jan 31, 2018

I'll make a separate ticket about the RestartSink

@ktoso

This comment has been minimized.

Show comment
Hide comment
@ktoso

ktoso Jan 31, 2018

Member

Sink ticket here: #24451
Closing this one for Flow as solved

Member

ktoso commented Jan 31, 2018

Sink ticket here: #24451
Closing this one for Flow as solved

@ktoso ktoso closed this Jan 31, 2018

@ktoso ktoso removed the 3 - in progress label Jan 31, 2018

@ktoso ktoso added this to the 2.5.10 milestone Jan 31, 2018

manonthegithub added a commit to manonthegithub/akka that referenced this issue Jan 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment