fix: fix lastAttempt value on first try when maxAttempt is zero #1397
Conversation
scrocquesel
changed the title
| @@ -342,6 +350,16 @@ public UpdateControl<ConfigMap> reconcile(ConfigMap resource, Context<ConfigMap> | |||
| } | |||
| } | |||
|
|
|||
| @ControllerConfiguration(retry = NoRetry.class) | |||
There was a problem hiding this comment.
@csviri Tried something with Optional but as expected, configuration is a bit weird. We need a marker class because the given class is not a factory but the actual strategy. Then, there is something wrong because nothing prevent from adding @GradualRetry with NoRetry.class.
Does something like
@ControllerConfiguration(retry = @GradualRetry(maxAttempts = 0))
@ControllerConfiguration(retry = @NoRetry)
with the annotation being a factory returning an Optional based on the given configuration could be better ?
I think this has been discussed already when introducing GradualRetry annotation in #1317 and I think we hit some limitations.
Maybe we can keep the annotation out of controllerConfiguration but need a way to discover annotations that would configure the controllerConfiguration and let them override default configuration.
@GradualRetry(maxAttempts = 0)
@ControllerConfiguration() // retry is no longer exposed
@NoRetry
@ControllerConfiguration() // retry is no longer exposed
| return controller.getConfiguration().getRetry() == null; | ||
| // on first try, we can only rely on the configured behavior | ||
| // if enabled, will at least produce one RetryExecution | ||
| return !controller.getConfiguration().getRetry().isPresent(); |
There was a problem hiding this comment.
This needs to be optimized to avoid multiple call to configuration doing initialization each time
|
@scrocquesel what is the state of this PR? Do you intend to continue with this, in that case probably should be rebased. Or I can take a look on the original issue. |
|
Jus out of curiosity why wouldn't you use a retry? |
…rk#1518) Co-authored-by: Chris Laprun <[email protected]>
* feat: decouple event source from cache + list discriminator (operator-framework#1378) * feat: bulk dependent resources (operator-framework#1448) * feat: optional eventsource on dependent resources (operator-framework#1479) * refactor: simplify handling of reused event sources (operator-framework#1518) Co-authored-by: Chris Laprun <[email protected]> * refactor: isolate index handling to BulkDependentResource interface (operator-framework#1517) * feat: key based bulk resource creation (operator-framework#1521) * improvement: bulk dependent resource api * merge Co-authored-by: Chris Laprun <[email protected]> Co-authored-by: Chris Laprun <[email protected]>
…ork#1544) * refactor: deleteBulkResource -> deleteTargetResource * refactor: remove now unneeded class * refactor: deleteBulkResourcesIfRequired -> deleteExtraResources * fix: javadoc
Generally, for testing purpose or when you set periodic reconciliation to lower impact on external services |
Help is welcome, I tried different things but i'm not satisfied. |
csviri
force-pushed
the
next
branch
2 times, most recently
from
0cf4dd3 to
eafe869
Compare
|
I think the next branch deletion, closed this. It clearly was not intentional. |
|
Pls reopen it if you want to proceed. If not I can take a look on this issue. |
You may cherry pick d500347 to have a red test to start with. |
Fix #1395
I start adding
enabled()on Retry interface but it feels wrong. Retry is a functional interface so it needs a default and neither true or false is a good choice. Moreover, implementation can wrongly implement it. For eg, return true while not returning at least one next delay.Seems like ReconciliationDispatcher may call initExecution but if we don't want to create an instance only to test nextDelay result, it should be propagated up to the EventProcessor to be cached and I don't see how to do that.