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.

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

Restart(Source|Flow|Sink): Configurable stream restart deadline #29591

Merged
merged 11 commits into from Oct 5, 2020

Conversation

r-glyde
Copy link
Contributor

@r-glyde r-glyde commented Sep 11, 2020

References #29291 (and sort of #29550)

Docs could probably also do with a bit of an update after this but was waiting to see general thoughts first.

Currently introducing additional overloaded methods (except for the deprecated javadsl) to try to keep binary compatibility but there are now quite a lot of these methods hanging around which might make this harder to follow. Another option could be to introduce a settings class for these sort of options. Probably wouldn't make much difference in this PR as we would still need keep existing methods and introduce new ones to receive the settings class but it might make future changes easier? Let me know what's preferred and happy to update.


Edit - dc3c1e7 includes creating RestartSettings to hold configuration which can then be passed as a whole to the actual restart methods. I think this ends up looking a bit tidier and expect it should be easier to add additional configuration options in the future if needed. I'm not sure what anyone thinks about exactly what config options should be part of this class but for now I've just put everything except onlyOnFailures (I quite like having the separate withBackoff and onFailuresWithBackoff functions).

Let me know if the original API is preferred and I can just remove that commit.

* use resetDeadline over minBackoff
* scala and java overloads
@akka-ci
Copy link

akka-ci commented Sep 11, 2020

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!

@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 Sep 11, 2020
@akka-ci
Copy link

akka-ci commented Sep 11, 2020

Test PASSed.

1 similar comment
@akka-ci
Copy link

akka-ci commented Sep 11, 2020

Test PASSed.

@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 Sep 12, 2020
@akka-ci
Copy link

akka-ci commented Sep 12, 2020

Test PASSed.

@laszlovandenhoek
Copy link
Contributor

As to docs, see existing issue: #28178

It appears @vaslabs is another poor soul running into this :)

@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 Sep 15, 2020
@akka-ci
Copy link

akka-ci commented Sep 15, 2020

Test FAILed.

@r-glyde r-glyde force-pushed the configurable-restart-deadline branch from 66c9520 to bb4a523 Compare September 15, 2020 19:19
@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 needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Sep 15, 2020
@akka-ci
Copy link

akka-ci commented Sep 15, 2020

Test FAILed.

* document RestartSettings
* update operator docs
* reference BackoffOpts instead of Backoff
@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 Sep 30, 2020
@akka-ci
Copy link

akka-ci commented Sep 30, 2020

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 Sep 30, 2020
@akka-ci
Copy link

akka-ci commented Sep 30, 2020

Test PASSed.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

Re-read the descriptions and have a few more clarifying suggestions.

@@ -31,4 +40,6 @@ This uses the same exponential backoff algorithm as @apidoc[Backoff$].

**backpressures** during backoff and when the wrapped flow backpressures

**completes** when the wrapped flow completes or maxRestarts are reached
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
**completes** when the wrapped flow completes or maxRestarts are reached
**completes** when the wrapped flow completes or `maxRestarts` are reached within the given time limit

@@ -29,6 +39,6 @@ This uses the same exponential backoff algorithm as @apidoc[Backoff$].

**backpressures** during backoff and when the wrapped flow backpressures

**completes** when the wrapped flow completes
**completes** when maxRestarts are reached
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**completes** when maxRestarts are reached
**completes** when `maxRestarts` are reached within the given time limit

completed. Any termination by the @apidoc[Flow] before that time will be handled by restarting it. Any termination
signals sent to this @apidoc[Flow] however will terminate the wrapped @apidoc[Flow], if it's running, and then the @apidoc[Flow]
will be allowed to terminate without being restarted.
Wrap the given @apidoc[Flow] with a @apidoc[Flow] that will restart it when it fails or complete using an exponential backoff.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Wrap the given @apidoc[Flow] with a @apidoc[Flow] that will restart it when it fails or complete using an exponential backoff.
Wrap the given @apidoc[Flow] with a @apidoc[Flow] that will restart it when it completes or fails using exponential backoff.

happens, the @apidoc[Sink], if currently running, will terminate and will not be restarted. This can be triggered
simply by the upstream completing, or externally by introducing a @apidoc[KillSwitch] right before this @apidoc[Sink] in the
graph.
Wrap the given @apidoc[Sink] with a @apidoc[Sink] that will restart it when it fails or complete using an exponential backoff.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Wrap the given @apidoc[Sink] with a @apidoc[Sink] that will restart it when it fails or complete using an exponential backoff.
Wrap the given @apidoc[Sink] with a @apidoc[Sink] that will restart it when it completes or fails using exponential backoff.

* @ref:[RestartSource.withBackoff](../RestartSource/withBackoff.md)
* @ref:[RestartSource.onFailuresWithBackoff](../RestartSource/onFailuresWithBackoff.md)
* @ref:[RestartFlow.onFailuresWithBackoff](../RestartFlow/onFailuresWithBackoff.md)
* @ref:[RestartFlow.withBackoff](../RestartFlow/withBackoff.md)
Copy link
Member

Choose a reason for hiding this comment

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

This seems to lack a Reactive Streams semantics section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yeah! Sorry, went through updating the others and just forgot that this one didn't exist 🙂

@@ -61,4 +64,10 @@ Java

**emits** when the wrapped source emits

**backpressures** during backoff and when downstream backpressures

**completes** when the wrapped source completes or maxRestarts is reached
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
**completes** when the wrapped source completes or maxRestarts is reached
**completes** when the wrapped source completes or maxRestarts is reached within the given time limit

@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 Oct 2, 2020
@akka-ci
Copy link

akka-ci commented Oct 2, 2020

Test PASSed.

Copy link
Member

@ennru ennru left a comment

Choose a reason for hiding this comment

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

LGTM.

@ennru ennru added this to the 2.6.10 milestone Oct 5, 2020
@ennru ennru changed the title Configurable stream restart deadline Restart(Source|Flow|Sink): Configurable stream restart deadline Oct 5, 2020
@ennru ennru merged commit a4acf23 into akka:master Oct 5, 2020
@ennru
Copy link
Member

ennru commented Oct 5, 2020

Thank you so much for this work @r-glydem, highly appriciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
t:stream tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants