Retry Support #252

csviri · 2020-12-08T14:49:14Z

This PR introduces the retry support back for new core with event sources.

# Conflicts: # operator-framework/src/main/java/io/javaoperatorsdk/operator/Operator.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/EventDispatcher.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionConsumer.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/PostExecutionControl.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/retry/GenericRetry.java # operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/retry/GenericRetryExecution.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/EventDispatcherTest.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManagerTest.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/processing/retry/GenericRetryExecutionTest.java

s-soroosh · 2020-12-11T15:24:38Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/RetryInfo.java

@@ -2,19 +2,24 @@

 public class RetryInfo {


What about turning it into an interface extended by RetryExecution and implemented by GenericRetryExecution?
so the execution can be returned as info and no need to instantiate a new RetryInfo on every call.

Hehe, I like to work with Pojos, but will check how that would look like, I change it in case its nice.

metacosm

I will try to look at things more closely over the week-end.

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/RetryInfo.java

s-soroosh · 2020-12-11T16:13:44Z

operator-framework/src/main/java/io/javaoperatorsdk/operator/processing/ExecutionConsumer.java

@@ -22,7 +22,7 @@

  @Override
  public void run() {
-    PostExecutionControl postExecutionControl = eventDispatcher.handleEvent(executionScope);
+    PostExecutionControl postExecutionControl = eventDispatcher.handleExecution(executionScope);


just loud thinking, wouldn't the code be simpler if we had 2 ExecutionConsumers called
SimpleConsumer and RetryableConsumer and let the consumer to take care of the retrying rather than the handler?

@psycho-ir thing is this is closely relates with the events are comming in while the controller is executed, that layer does not have enough info basically now. (Also its just really a wraper around executor).

csviri · 2020-12-11T16:39:45Z

Will make the changes an fix this test next week.

adam-sandor · 2020-12-14T13:48:46Z

How does the user configure the retries?

csviri · 2020-12-14T14:18:31Z

How does the user configure the retries?

Just as before, when registering the controller.

csviri · 2020-12-14T14:50:13Z

@adam-sandor @metacosm @psycho-ir and ALL. This is the MVP of the Retry. This works in a way that if there is an exception thrown from the controller, we will schedule a timer event to retry the execution. (Also we are putting back the events, which were in the scope of the current execution). The retry can be configured when registering the controller.

Note that there are all kind of situations which can happen, like an event is received while there is already a reatry planned or a new event is received when there is already the max attempt reached of a retry. In all these situations we execute the controller. So basically we are not blocking the execution because of an ongoing retry. This is something that we might discuss and change in the future. I think its not straightforward how this should go.

Maybe it would be more intuitive behavior that if there is already a scheduled event for retry, we don't execute the controller for new events just for the retry event. But this is up to discussion.

Pls let me know if you have any questions, we can also go through the impl. Otherwise would like to merge it.

metacosm · 2020-12-14T14:57:01Z

Note that there are all kind of situations which can happen, like an event is received while there is already a reatry planned or a new event is received when there is already the max attempt reached of a retry. In all these situations we execute the controller. So basically we are not blocking the execution because of an ongoing retry. This is something that we might discuss and change in the future. I think its not straightforward how this should go.

Can you give a more concrete example of what you mean here? What are the different options?

Maybe it would be more intuitive behavior that if there is already a scheduled event for retry, we don't execute the controller for new events just for the retry event. But this is up to discussion.

The thing that I'm not clear about is what these "new events" are? If they are events from the cluster (i.e. there was a change in the resource state on the cluster), whatever the controller is doing should probably be stopped to try to reconcile the new state instead.

s-soroosh · 2020-12-14T16:47:36Z

Maybe it would be more intuitive behavior that if there is already a scheduled event for retry, we don't execute the controller for new events just for the retry event. But this is up to discussion.

I would say the current implementation is more reasonable, stop reconciling on new events may lead to stop the world for a while just because of a runtime exception happened.

The changes LGTM, What IMHO is crucial is documenting this feature and also the event sources via javadoc ASAP.

Mallington · 2020-12-14T22:37:24Z

Looks Great, this is exactly what I was looking for. Are you planning on creating a new release tag with these changes? I plan to uptake them once they are released.
Kind regards.

s-soroosh · 2020-12-14T22:41:25Z

Looks Great, this is exactly what I was looking for. Are you planning on creating a new release tag with these changes? I plan to uptake them once they are released.
Kind regards.

You should already be able to play around with these changes by changing your dependency version to 1.4.1-SNAPSHOT,
We will do the actual release of 1.5.0 very soon (most probably tomorrow).

jfgosselin · 2020-12-16T19:24:12Z

Hi,

How/where is the maxElapsedTime used ? :

https://github.com/java-operator-sdk/java-operator-sdk/blob/v1.5.0/spring-boot-starter/src/main/java/io/javaoperatorsdk/operator/springboot/starter/RetryProperties.java#L12

csviri · 2020-12-17T08:53:42Z

@jfgosselin this is a leftover, should not be there, will remove it, thx!

retry basic impl

1883727

csviri changed the title ~~retry basic impl~~ Retry Support Dec 8, 2020

csviri changed the title ~~Retry Support~~ Retry Support (WIP) Dec 8, 2020

csviri added 5 commits December 9, 2020 09:49

merged master, google formatter

95450aa

fix, operator api update for retry

4511728

retry tests, fixes

115e44d

retry improvements and tests

10c97e1

csviri changed the title ~~Retry Support (WIP)~~ Retry Support Dec 11, 2020

s-soroosh reviewed Dec 11, 2020

View reviewed changes

metacosm reviewed Dec 11, 2020

View reviewed changes

operator-framework/src/main/java/io/javaoperatorsdk/operator/api/RetryInfo.java Outdated Show resolved Hide resolved

s-soroosh and others added 2 commits December 11, 2020 16:55

Merge branch 'master' into retry-experiment

13da52f

resolve conflict

1affee4

s-soroosh reviewed Dec 11, 2020

View reviewed changes

csviri added 3 commits December 14, 2020 14:03

renaming from PR comments, test info message

b1d9f1d

improved logging

6ac3ad0

retry test fix

4850606

test fix

f30f35e

csviri requested review from metacosm, s-soroosh and adam-sandor December 14, 2020 14:51

s-soroosh merged commit 62ea9f6 into master Dec 14, 2020

s-soroosh deleted the retry-experiment branch December 14, 2020 21:11

Retry Support #252

Retry Support #252

Uh oh!

Conversation

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

Uh oh!

Uh oh!

s-soroosh Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

csviri Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

metacosm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

s-soroosh Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

csviri Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

csviri commented Dec 11, 2020

Uh oh!

adam-sandor commented Dec 14, 2020

Uh oh!

csviri commented Dec 14, 2020

Uh oh!

csviri commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metacosm commented Dec 14, 2020

Uh oh!

s-soroosh commented Dec 14, 2020

Uh oh!

Mallington commented Dec 14, 2020

Uh oh!

s-soroosh commented Dec 14, 2020

Uh oh!

jfgosselin commented Dec 16, 2020

Uh oh!

csviri commented Dec 17, 2020

Uh oh!

Uh oh!

csviri commented Dec 8, 2020 •

edited

Loading

csviri commented Dec 14, 2020 •

edited

Loading