feat: decouple configuration of rate limiting and retry from controller #1317
Conversation
...ework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java
Outdated
Show resolved
Hide resolved
metacosm
changed the title
There was a problem hiding this comment.
I have mixed feelings about this concept (well that includes positive ones :) ) , for now we had every configuration in @ControllerConfiguration configutation, here some of the configuration would be not part of it but a separate annotation. It makes sense if users want to configure own implementation of these concepts (retry, rate limit). However the default implementations should cover most of the cases. The extensibility is mostly there for sake of generality.
I'm not opposing, I see what problem this solves, just pointing out that separate annotations, are subject of developers annoyance, since you have to learn this feature from the docs, not directly visible from the code.
Therefore we have to document this very carefully both in docs and javadoc
...-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/RetryConfiguration.java
Show resolved
Hide resolved
…va-operator-sdk into clean-configuration
.../src/main/java/io/javaoperatorsdk/operator/processing/event/rate/LimitingRateOverPeriod.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Optional list of {@link Dependent} configurations which associate a resource type to a | ||
| * {@link io.javaoperatorsdk.operator.api.reconciler.dependent.DependentResource} implementation | ||
| * | ||
| * @return the list of {@link Dependent} configurations | ||
| */ | ||
| Dependent[] dependents() default {}; | ||
|
|
||
| Class<? extends Retry> retry() default GenericRetry.class; |
There was a problem hiding this comment.
Maybe have some javadoc here? link how to configur it.
...e/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java
Show resolved
Hide resolved
...-core/src/main/java/io/javaoperatorsdk/operator/processing/event/rate/PeriodRateLimiter.java
Outdated
Show resolved
Hide resolved
| this.limitForPeriod = configuration.maxReconciliations(); | ||
| } | ||
|
|
||
| public boolean isActivated() { |
There was a problem hiding this comment.
Don;t think this needs to be exposed, thus should not be public.
There was a problem hiding this comment.
It's used in a test in a different package so no choice… :(
...ework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/RetryingGradually.java
Outdated
Show resolved
Hide resolved
|
Addressed comments. |
|
Kudos, SonarCloud Quality Gate passed! |
…er (#1317) Co-authored-by: csviri <[email protected]>
…er (#1317) Co-authored-by: csviri <[email protected]>
…er (#1317) Co-authored-by: csviri <[email protected]>
…er (#1317) Co-authored-by: csviri <[email protected]>
No description provided.