feature: controller reconciliation max delay #871
Conversation
| @@ -114,4 +115,12 @@ default boolean useFinalizer() { | |||
| default ResourceEventFilter<R> getEventFilter() { | |||
| return ResourceEventFilters.passthrough(); | |||
| } | |||
|
|
|||
| default long reconciliationMaxInterval() { | |||
There was a problem hiding this comment.
I think it will be better to introduce a separate annotation if we split the duration and its unit to make the name simpler and make it more coherent because it doesn't make sense to provide one without the other.
There was a problem hiding this comment.
I'm a little worried that it would unnecessary complicate things; also there is always a default value. Also both start with same prefix, I don't think this will be confusing.
There was a problem hiding this comment.
An alternative is to have the time as a string, like "10h" or "140m". But we use this separation in other API already.
There was a problem hiding this comment.
I don't think it would complicate things and it would make things cleaner to group related things in a single part. Otherwise, you could have annotated reconcilers with just reconciliationMaxIntervalTimeUnit and that would look really weird. Currently, the only hint that these things are related are their names. It's better to make it explicit, in my opinion. It also avoids polluting the main configuration with things that will rarely be used. Embedding the unit in the String is a recipe for disaster, imo, and I think it's much cleaner to separate the configuration into a separate annotation.
There was a problem hiding this comment.
So just to see it clear. The reason to separate this into a new annotation because there are two related fields?
I'm not sure that makes the effort, especially since we everything in one (for now mandatory) annotation what is a good thing.
We would like to have this feature by default turned on, so what if that annotation is not present? Actually I guess should be turned off? It's easy to miss that it even exists.
There was a problem hiding this comment.
Maybe the best would be to ask the users. @andreaTP @lburgazzoli what do you think, should be this a separate annotation or not?
see also the issue:
#848
Thank you.
There was a problem hiding this comment.
The namespace control is done that way. Pretty much all the annotation fields have default values so we already turn things on without the annotation being present. This would be the exact same thing here: the child annotation would just allow people to change the default value.
There was a problem hiding this comment.
To be clear, I'm not saying we should have a completely separate annotation, just a composite child annotation for this.
There was a problem hiding this comment.
To be clear, I'm not saying we should have a completely separate annotation, just a composite child annotation for this.
Ahhh, ok that is a completely different story! Sure that makes sense! Sorry....
There was a problem hiding this comment.
I only understood that I hadn't been clear enough with what I was saying with your last comment :)
Sorry! 😓
Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
docs/documentation/features.md
Outdated
| the standard annotation: | ||
|
|
||
| ```java | ||
| @ControllerConfiguration(finalizerName = NO_FINALIZER, reconciliationMaxInterval = 2L, |
There was a problem hiding this comment.
This needs to be updated.
|
Kudos, SonarCloud Quality Gate passed! |
No description provided.