Skip to content

feat: retry and retry configuration decoupling #1285

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

Merged
merged 3 commits into from
Jun 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter;
import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilters;
import io.javaoperatorsdk.operator.processing.retry.GenericRetry;
import io.javaoperatorsdk.operator.processing.retry.Retry;

public interface ControllerConfiguration<R extends HasMetadata> extends ResourceConfiguration<R> {

Expand All @@ -27,6 +29,16 @@ default boolean isGenerationAware() {

String getAssociatedReconcilerClassName();

default Retry getRetry() {
return GenericRetry.fromConfiguration(getRetryConfiguration()); // NOSONAR
}

/**
* Use getRetry instead.
*
* @return configuration for retry.
*/
@Deprecated
default RetryConfiguration getRetryConfiguration() {
return RetryConfiguration.DEFAULT;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
import io.javaoperatorsdk.operator.processing.dependent.kubernetes.KubernetesDependentResourceConfig;
import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter;
import io.javaoperatorsdk.operator.processing.retry.GenericRetry;
import io.javaoperatorsdk.operator.processing.retry.Retry;

import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_NAMESPACES_SET;
import static io.javaoperatorsdk.operator.api.reconciler.Constants.WATCH_CURRENT_NAMESPACE_SET;
Expand All @@ -19,7 +21,7 @@ public class ControllerConfigurationOverrider<R extends HasMetadata> {
private String finalizer;
private boolean generationAware;
private Set<String> namespaces;
private RetryConfiguration retry;
private Retry retry;
private String labelSelector;
private ResourceEventFilter<R> customResourcePredicate;
private final ControllerConfiguration<R> original;
Expand All @@ -30,7 +32,7 @@ private ControllerConfigurationOverrider(ControllerConfiguration<R> original) {
finalizer = original.getFinalizerName();
generationAware = original.isGenerationAware();
namespaces = new HashSet<>(original.getNamespaces());
retry = original.getRetryConfiguration();
retry = original.getRetry();
labelSelector = original.getLabelSelector();
customResourcePredicate = original.getEventFilter();
reconciliationMaxInterval = original.reconciliationMaxInterval().orElse(null);
Expand Down Expand Up @@ -90,11 +92,17 @@ public ControllerConfigurationOverrider<R> watchingAllNamespaces() {
return this;
}

public ControllerConfigurationOverrider<R> withRetry(RetryConfiguration retry) {
public ControllerConfigurationOverrider<R> withRetry(Retry retry) {
this.retry = retry;
return this;
}

@Deprecated
public ControllerConfigurationOverrider<R> withRetry(RetryConfiguration retry) {
this.retry = GenericRetry.fromConfiguration(retry);
return this;
}

public ControllerConfigurationOverrider<R> withLabelSelector(String labelSelector) {
this.labelSelector = labelSelector;
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.dependent.DependentResourceSpec;
import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEventFilter;
import io.javaoperatorsdk.operator.processing.retry.Retry;

@SuppressWarnings("rawtypes")
public class DefaultControllerConfiguration<R extends HasMetadata>
Expand All @@ -20,7 +21,7 @@ public class DefaultControllerConfiguration<R extends HasMetadata>
private final String crdName;
private final String finalizer;
private final boolean generationAware;
private final RetryConfiguration retryConfiguration;
private final Retry retry;
private final ResourceEventFilter<R> resourceEventFilter;
private final List<DependentResourceSpec> dependents;
private final Duration reconciliationMaxInterval;
Expand All @@ -33,7 +34,7 @@ public DefaultControllerConfiguration(
String finalizer,
boolean generationAware,
Set<String> namespaces,
RetryConfiguration retryConfiguration,
Retry retry,
String labelSelector,
ResourceEventFilter<R> resourceEventFilter,
Class<R> resourceClass,
Expand All @@ -46,10 +47,10 @@ public DefaultControllerConfiguration(
this.finalizer = finalizer;
this.generationAware = generationAware;
this.reconciliationMaxInterval = reconciliationMaxInterval;
this.retryConfiguration =
retryConfiguration == null
? ControllerConfiguration.super.getRetryConfiguration()
: retryConfiguration;
this.retry =
retry == null
? ControllerConfiguration.super.getRetry()
: retry;
this.resourceEventFilter = resourceEventFilter;

this.dependents = dependents != null ? dependents : Collections.emptyList();
Expand Down Expand Up @@ -81,8 +82,8 @@ public String getAssociatedReconcilerClassName() {
}

@Override
public RetryConfiguration getRetryConfiguration() {
return retryConfiguration;
public Retry getRetry() {
return retry;
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceAction;
import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceEvent;
import io.javaoperatorsdk.operator.processing.event.source.timer.TimerEventSource;
import io.javaoperatorsdk.operator.processing.retry.GenericRetry;
import io.javaoperatorsdk.operator.processing.retry.Retry;
import io.javaoperatorsdk.operator.processing.retry.RetryExecution;

Expand Down Expand Up @@ -54,8 +53,7 @@ class EventProcessor<R extends HasMetadata> implements EventHandler, LifecycleAw
ExecutorServiceManager.instance().executorService(),
eventSourceManager.getController().getConfiguration().getName(),
new ReconciliationDispatcher<>(eventSourceManager.getController()),
GenericRetry.fromConfiguration(
eventSourceManager.getController().getConfiguration().getRetryConfiguration()),
eventSourceManager.getController().getConfiguration().getRetry(),
ConfigurationServiceProvider.instance().getMetrics(),
eventSourceManager);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,7 +178,7 @@ public int getAttemptCount() {

@Override
public boolean isLastAttempt() {
return controller.getConfiguration().getRetryConfiguration() == null;
return controller.getConfiguration().getRetry() == null;
}
});
((DefaultContext<R>) context).setRetryInfo(retryInfo);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
import io.javaoperatorsdk.operator.api.config.RetryConfiguration;

public class GenericRetry implements Retry {
private int maxAttempts = DEFAULT_MAX_ATTEMPTS;
private long initialInterval = DEFAULT_INITIAL_INTERVAL;
private double intervalMultiplier = DEFAULT_MULTIPLIER;
private int maxAttempts = RetryConfiguration.DEFAULT_MAX_ATTEMPTS;
private long initialInterval = RetryConfiguration.DEFAULT_INITIAL_INTERVAL;
private double intervalMultiplier = RetryConfiguration.DEFAULT_MULTIPLIER;
private long maxInterval = -1;

public static GenericRetry defaultLimitedExponentialRetry() {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
package io.javaoperatorsdk.operator.processing.retry;

import io.javaoperatorsdk.operator.api.config.RetryConfiguration;

public interface Retry extends RetryConfiguration {
public interface Retry {

RetryExecution initExecution();

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import org.slf4j.LoggerFactory;

import io.fabric8.kubernetes.api.model.HasMetadata;
import io.javaoperatorsdk.operator.api.config.RetryConfiguration;
import io.javaoperatorsdk.operator.api.monitoring.Metrics;
import io.javaoperatorsdk.operator.processing.event.source.controller.ControllerResourceEventSource;
import io.javaoperatorsdk.operator.processing.event.source.controller.ResourceAction;
Expand Down Expand Up @@ -104,7 +105,7 @@ void schedulesAnEventRetryOnException() {
eventProcessorWithRetry.eventProcessingFinished(executionScope, postExecutionControl);

verify(retryTimerEventSourceMock, times(1))
.scheduleOnce(eq(customResource), eq(GenericRetry.DEFAULT_INITIAL_INTERVAL));
.scheduleOnce(eq(customResource), eq(RetryConfiguration.DEFAULT_INITIAL_INTERVAL));
}

@Test
Expand Down Expand Up @@ -135,7 +136,7 @@ void executesTheControllerInstantlyAfterErrorIfNewEventsReceived() {
List<ExecutionScope> allValues = executionScopeArgumentCaptor.getAllValues();
assertThat(allValues).hasSize(2);
verify(retryTimerEventSourceMock, never())
.scheduleOnce(eq(customResource), eq(GenericRetry.DEFAULT_INITIAL_INTERVAL));
.scheduleOnce(eq(customResource), eq(RetryConfiguration.DEFAULT_INITIAL_INTERVAL));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
import io.javaoperatorsdk.operator.api.config.ConfigurationServiceProvider;
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.api.config.MockControllerConfiguration;
import io.javaoperatorsdk.operator.api.config.RetryConfiguration;
import io.javaoperatorsdk.operator.api.reconciler.Cleaner;
import io.javaoperatorsdk.operator.api.reconciler.Context;
import io.javaoperatorsdk.operator.api.reconciler.DeleteControl;
Expand All @@ -36,6 +35,7 @@
import io.javaoperatorsdk.operator.api.reconciler.UpdateControl;
import io.javaoperatorsdk.operator.processing.Controller;
import io.javaoperatorsdk.operator.processing.event.ReconciliationDispatcher.CustomResourceFacade;
import io.javaoperatorsdk.operator.processing.retry.GenericRetry;
import io.javaoperatorsdk.operator.sample.observedgeneration.ObservedGenCustomResource;
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;

Expand Down Expand Up @@ -110,7 +110,7 @@ private <R extends HasMetadata> ReconciliationDispatcher<R> init(R customResourc
when(configuration.getFinalizerName()).thenReturn(DEFAULT_FINALIZER);
when(configuration.getName()).thenReturn("EventDispatcherTestController");
when(configuration.getResourceClass()).thenReturn(resourceClass);
when(configuration.getRetryConfiguration()).thenReturn(RetryConfiguration.DEFAULT);
when(configuration.getRetry()).thenReturn(new GenericRetry());
when(configuration.reconciliationMaxInterval())
.thenReturn(Optional.of(Duration.ofHours(RECONCILIATION_MAX_INTERVAL)));

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,33 +4,36 @@

import org.junit.jupiter.api.Test;

import static io.javaoperatorsdk.operator.processing.retry.GenericRetry.DEFAULT_INITIAL_INTERVAL;
import io.javaoperatorsdk.operator.api.config.RetryConfiguration;

import static org.assertj.core.api.Assertions.assertThat;

public class GenericRetryExecutionTest {

@Test
public void forFirstBackOffAlwaysReturnsInitialInterval() {
assertThat(getDefaultRetryExecution().nextDelay().get()).isEqualTo(DEFAULT_INITIAL_INTERVAL);
assertThat(getDefaultRetryExecution().nextDelay().get())
.isEqualTo(RetryConfiguration.DEFAULT_INITIAL_INTERVAL);
}

@Test
public void delayIsMultipliedEveryNextDelayCall() {
RetryExecution retryExecution = getDefaultRetryExecution();

Optional<Long> res = callNextDelayNTimes(retryExecution, 1);
assertThat(res.get()).isEqualTo(DEFAULT_INITIAL_INTERVAL);
assertThat(res.get()).isEqualTo(RetryConfiguration.DEFAULT_INITIAL_INTERVAL);

res = retryExecution.nextDelay();
assertThat(res.get())
.isEqualTo((long) (DEFAULT_INITIAL_INTERVAL * GenericRetry.DEFAULT_MULTIPLIER));
.isEqualTo((long) (RetryConfiguration.DEFAULT_INITIAL_INTERVAL
* RetryConfiguration.DEFAULT_MULTIPLIER));

res = retryExecution.nextDelay();
assertThat(res.get())
.isEqualTo(
(long) (DEFAULT_INITIAL_INTERVAL
* GenericRetry.DEFAULT_MULTIPLIER
* GenericRetry.DEFAULT_MULTIPLIER));
(long) (RetryConfiguration.DEFAULT_INITIAL_INTERVAL
* RetryConfiguration.DEFAULT_MULTIPLIER
* RetryConfiguration.DEFAULT_MULTIPLIER));
}

@Test
Expand Down