refactor: allow optional retry

scrocquesel · scrocquesel · commit cf9793659e61 · 2022-10-16T15:18:24.000+02:00
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java
@@ -30,6 +30,7 @@
 import io.javaoperatorsdk.operator.processing.event.source.filter.OnAddFilter;
 import io.javaoperatorsdk.operator.processing.event.source.filter.OnDeleteFilter;
 import io.javaoperatorsdk.operator.processing.event.source.filter.OnUpdateFilter;
+import io.javaoperatorsdk.operator.processing.retry.NoRetry;
 import io.javaoperatorsdk.operator.processing.retry.Retry;
 
 import static io.javaoperatorsdk.operator.api.reconciler.Constants.DEFAULT_NAMESPACES_SET;
@@ -150,10 +151,14 @@ public RateLimiter getRateLimiter() {
   }
 
   @Override
-  public Retry getRetry() {
+  public Optional<Retry> getRetry() {
     final Class<? extends Retry> retryClass = annotation.retry();
-    return Utils.instantiateAndConfigureIfNeeded(retryClass, Retry.class,
-        Utils.contextFor(this, null, null), this::configureFromAnnotatedReconciler);
+    if (NoRetry.class.equals(retryClass)) {
+      return Optional.empty();
+    }
+
+    return Optional.of(Utils.instantiateAndConfigureIfNeeded(retryClass, Retry.class,
+        Utils.contextFor(this, null, null), this::configureFromAnnotatedReconciler));
   }
 
 
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfiguration.java
@@ -36,11 +36,11 @@ default boolean isGenerationAware() {
 
   String getAssociatedReconcilerClassName();
 
-  default Retry getRetry() {
+  default Optional<Retry> getRetry() {
     final var configuration = getRetryConfiguration();
-    return !RetryConfiguration.DEFAULT.equals(configuration)
+    return Optional.of(!RetryConfiguration.DEFAULT.equals(configuration)
         ? GenericRetry.fromConfiguration(configuration)
-        : GenericRetry.DEFAULT; // NOSONAR
+        : GenericRetry.DEFAULT); // NOSONAR
   }
 
   /**
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverrider.java
@@ -29,7 +29,7 @@ public class ControllerConfigurationOverrider<R extends HasMetadata> {
   private String finalizer;
   private boolean generationAware;
   private Set<String> namespaces;
-  private Retry retry;
+  private Optional<Retry> retry;
   private String labelSelector;
   private ResourceEventFilter<R> customResourcePredicate;
   private final ControllerConfiguration<R> original;
@@ -113,12 +113,17 @@ public ControllerConfigurationOverrider<R> watchingAllNamespaces() {
    */
   @Deprecated(forRemoval = true)
   public ControllerConfigurationOverrider<R> withRetry(RetryConfiguration retry) {
-    this.retry = GenericRetry.fromConfiguration(retry);
+    this.retry = Optional.of(GenericRetry.fromConfiguration(retry));
     return this;
   }
 
   public ControllerConfigurationOverrider<R> withRetry(Retry retry) {
-    this.retry = retry;
+    this.retry = Optional.of(retry);
+    return this;
+  }
+
+  public ControllerConfigurationOverrider<R> withoutRetry() {
+    this.retry = Optional.empty();
     return this;
   }
 
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/DefaultControllerConfiguration.java
@@ -26,7 +26,7 @@ public class DefaultControllerConfiguration<R extends HasMetadata>
   private final String crdName;
   private final String finalizer;
   private final boolean generationAware;
-  private final Retry retry;
+  private final Optional<Retry> retry;
   private final ResourceEventFilter<R> resourceEventFilter;
   private final List<DependentResourceSpec> dependents;
   private final Duration reconciliationMaxInterval;
@@ -40,7 +40,7 @@ public DefaultControllerConfiguration(
       String finalizer,
       boolean generationAware,
       Set<String> namespaces,
-      Retry retry,
+      Optional<Retry> retry,
       String labelSelector,
       ResourceEventFilter<R> resourceEventFilter,
       Class<R> resourceClass,
@@ -93,7 +93,7 @@ public String getAssociatedReconcilerClassName() {
   }
 
   @Override
-  public Retry getRetry() {
+  public Optional<Retry> getRetry() {
     return retry;
   }
 
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventProcessor.java
@@ -40,7 +40,7 @@ public class EventProcessor<R extends HasMetadata> implements EventHandler, Life
   private volatile boolean running;
   private final ControllerConfiguration<?> controllerConfiguration;
   private final ReconciliationDispatcher<R> reconciliationDispatcher;
-  private final Retry retry;
+  private final Optional<Retry> retry;
   private final ExecutorService executor;
   private final Metrics metrics;
   private final Cache<R> cache;
@@ -346,7 +346,7 @@ private ResourceState getOrInitRetryExecution(ExecutionScope<R> executionScope)
     final var state = resourceStateManager.getOrCreate(executionScope.getResourceID());
     RetryExecution retryExecution = state.getRetry();
     if (retryExecution == null) {
-      retryExecution = retry.initExecution();
+      retryExecution = retry.map(r -> r.initExecution()).orElseThrow();
       state.setRetry(retryExecution);
     }
     return state;
@@ -367,7 +367,7 @@ private void unsetUnderExecution(ResourceID resourceID) {
   }
 
   private boolean isRetryConfigured() {
-    return retry != null;
+    return retry.isPresent();
   }
 
   @Override
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcher.java
@@ -165,8 +165,19 @@ private PostExecutionControl<P> handleErrorStatusHandler(P resource, P originalR
       Exception e) throws Exception {
     if (isErrorStatusHandlerPresent()) {
       try {
-        RetryInfo retryInfo =
-            context.getRetryInfo().orElse(controller.getConfiguration().getRetry().initExecution());
+        RetryInfo retryInfo = context.getRetryInfo().orElse(new RetryInfo() {
+          @Override
+          public int getAttemptCount() {
+            return 0;
+          }
+
+          @Override
+          public boolean isLastAttempt() {
+            // on first try, we can only rely on the configured behavior
+            // if enabled, will at least produce one RetryExecution
+            return !controller.getConfiguration().getRetry().isPresent();
+          }
+        });
         ((DefaultContext<P>) context).setRetryInfo(retryInfo);
         var errorStatusUpdateControl = ((ErrorStatusHandler<P>) controller.getReconciler())
             .updateErrorStatus(resource, context, e);
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/GenericRetry.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/GenericRetry.java
@@ -15,6 +15,11 @@ public static GenericRetry defaultLimitedExponentialRetry() {
     return (GenericRetry) DEFAULT;
   }
 
+  /**
+   * @deprecated Use the {@link NoRetry} annotation instead or override configuration with
+   *             withoutRetry.
+   */
+  @Deprecated(forRemoval = true)
   public static GenericRetry noRetry() {
     return new GenericRetry().setMaxAttempts(0);
   }
@@ -94,6 +99,11 @@ public GenericRetry withLinearRetry() {
 
   @Override
   public void initFrom(GradualRetry configuration) {
+    if (configuration.maxAttempts() == 0) {
+      throw new IllegalArgumentException(
+          "maxAttempts must be non-zero. Use NoRetry to disable retry.");
+    }
+
     this.initialInterval = configuration.initialInterval();
     this.maxAttempts = configuration.maxAttempts();
     this.intervalMultiplier = configuration.intervalMultiplier();
diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/NoRetry.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/retry/NoRetry.java
@@ -0,0 +1,12 @@
+package io.javaoperatorsdk.operator.processing.retry;
+
+public class NoRetry implements Retry {
+
+  /* should not be instanciated */
+  private NoRetry() {}
+
+  @Override
+  public RetryExecution initExecution() {
+    return null;
+  }
+}
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventProcessorTest.java
@@ -73,13 +73,13 @@ void setup() {
     when(eventSourceManagerMock.getControllerResourceEventSource())
         .thenReturn(controllerResourceEventSourceMock);
     eventProcessor =
-        spy(new EventProcessor(controllerConfiguration(null, rateLimiterMock),
+        spy(new EventProcessor(controllerConfiguration(Optional.empty(), rateLimiterMock),
             reconciliationDispatcherMock,
             eventSourceManagerMock, null));
     eventProcessor.start();
     eventProcessorWithRetry =
         spy(new EventProcessor(
-            controllerConfiguration(GenericRetry.defaultLimitedExponentialRetry(),
+            controllerConfiguration(Optional.of(GenericRetry.defaultLimitedExponentialRetry()),
                 rateLimiterMock),
             reconciliationDispatcherMock, eventSourceManagerMock, null));
     eventProcessorWithRetry.start();
@@ -131,6 +131,20 @@ void schedulesAnEventRetryOnException() {
             eq(RetryConfiguration.DEFAULT_INITIAL_INTERVAL));
   }
 
+  @Test
+  void doNotSchedulesAnEventRetryOnExceptionIfNoRetry() {
+    TestCustomResource customResource = testCustomResource();
+
+    ExecutionScope executionScope = new ExecutionScope(customResource, null);
+    PostExecutionControl postExecutionControl =
+        PostExecutionControl.exceptionDuringExecution(new RuntimeException("test"));
+
+    eventProcessor.eventProcessingFinished(executionScope, postExecutionControl);
+
+    verify(retryTimerEventSourceMock, times(1))
+        .scheduleOnce(eq(ResourceID.fromResource(customResource)), eq(1000L));
+  }
+
   @Test
   void executesTheControllerInstantlyAfterErrorIfNewEventsReceived() {
     Event event = prepareCREvent();
@@ -264,7 +278,7 @@ void cancelScheduleOnceEventsOnSuccessfulExecution() {
   void startProcessedMarkedEventReceivedBefore() {
     var crID = new ResourceID("test-cr", TEST_NAMESPACE);
     eventProcessor =
-        spy(new EventProcessor(controllerConfiguration(null,
+        spy(new EventProcessor(controllerConfiguration(Optional.empty(),
             LinearRateLimiter.deactivatedRateLimiter()), reconciliationDispatcherMock,
             eventSourceManagerMock,
             metricsMock));
@@ -393,7 +407,7 @@ void schedulesRetryForMarReconciliationIntervalIfRetryExhausted() {
     Retry retry = mock(Retry.class);
     when(retry.initExecution()).thenReturn(mockRetryExecution);
     eventProcessorWithRetry =
-        spy(new EventProcessor(controllerConfiguration(retry,
+        spy(new EventProcessor(controllerConfiguration(Optional.of(retry),
             LinearRateLimiter.deactivatedRateLimiter()), reconciliationDispatcherMock,
             eventSourceManagerMock,
             metricsMock));
@@ -448,7 +462,7 @@ private void overrideData(ResourceID id, HasMetadata applyTo) {
     applyTo.getMetadata().setNamespace(id.getNamespace().orElse(null));
   }
 
-  ControllerConfiguration controllerConfiguration(Retry retry, RateLimiter rateLimiter) {
+  ControllerConfiguration controllerConfiguration(Optional<Retry> retry, RateLimiter rateLimiter) {
     ControllerConfiguration res = mock(ControllerConfiguration.class);
     when(res.getName()).thenReturn("Test");
     when(res.getRetry()).thenReturn(retry);
diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/ReconciliationDispatcherTest.java
@@ -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.getRetry()).thenReturn(new GenericRetry());
+    when(configuration.getRetry()).thenReturn(Optional.of(new GenericRetry()));
     when(configuration.maxReconciliationInterval())
         .thenReturn(Optional.of(Duration.ofHours(RECONCILIATION_MAX_INTERVAL)));
 
diff --git a/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java b/operator-framework-junit5/src/main/java/io/javaoperatorsdk/operator/junit/LocallyRunOperatorExtension.java
@@ -6,6 +6,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Optional;
 import java.util.function.Consumer;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
@@ -131,7 +132,7 @@ protected void before(ExtensionContext context) {
       final var oconfig = override(config).settingNamespace(namespace);
 
       if (ref.retry != null) {
-        oconfig.withRetry(ref.retry);
+        ref.retry.ifPresentOrElse(r -> oconfig.withRetry(r), () -> oconfig.withoutRetry());
       }
       if (ref.controllerConfigurationOverrider != null) {
         ref.controllerConfigurationOverrider.accept(oconfig);
@@ -192,7 +193,6 @@ protected void after(ExtensionContext context) {
     localPortForwards.clear();
   }
 
-  @SuppressWarnings("rawtypes")
   public static class Builder extends AbstractBuilder<Builder> {
     private final List<ReconcilerSpec> reconcilers;
     private final List<PortForwardSpec> portForwards;
@@ -208,30 +208,37 @@ protected Builder() {
 
     public Builder withReconciler(
         Reconciler value, Consumer<ControllerConfigurationOverrider> configurationOverrider) {
-      return withReconciler(value, null, configurationOverrider);
+      return withReconciler(value, (Retry) null, configurationOverrider);
     }
 
     public Builder withReconciler(
         Reconciler value,
         Retry retry,
         Consumer<ControllerConfigurationOverrider> configurationOverrider) {
+      return withReconciler(value, Optional.ofNullable(null), configurationOverrider);
+    }
+
+    public Builder withReconciler(
+        Reconciler value,
+        Optional<Retry> retry,
+        Consumer<ControllerConfigurationOverrider> configurationOverrider) {
       reconcilers.add(new ReconcilerSpec(value, retry, configurationOverrider));
       return this;
     }
 
-    @SuppressWarnings("rawtypes")
     public Builder withReconciler(Reconciler value) {
-      reconcilers.add(new ReconcilerSpec(value, null));
-      return this;
+      return withReconciler(value, (Retry) null);
     }
 
-    @SuppressWarnings("rawtypes")
     public Builder withReconciler(Reconciler value, Retry retry) {
+      return withReconciler(value, Optional.ofNullable(retry));
+    }
+
+    public Builder withReconciler(Reconciler value, Optional<Retry> retry) {
       reconcilers.add(new ReconcilerSpec(value, retry));
       return this;
     }
 
-    @SuppressWarnings("rawtypes")
     public Builder withReconciler(Class<? extends Reconciler> value) {
       try {
         reconcilers.add(new ReconcilerSpec(value.getConstructor().newInstance(), null));
@@ -313,16 +320,16 @@ public int getLocalPort() {
   @SuppressWarnings("rawtypes")
   private static class ReconcilerSpec {
     final Reconciler reconciler;
-    final Retry retry;
+    final Optional<Retry> retry;
     final Consumer<ControllerConfigurationOverrider> controllerConfigurationOverrider;
 
-    public ReconcilerSpec(Reconciler reconciler, Retry retry) {
+    public ReconcilerSpec(Reconciler reconciler, Optional<Retry> retry) {
       this(reconciler, retry, null);
     }
 
     public ReconcilerSpec(
         Reconciler reconciler,
-        Retry retry,
+        Optional<Retry> retry,
         Consumer<ControllerConfigurationOverrider> controllerConfigurationOverrider) {
       this.reconciler = reconciler;
       this.retry = retry;
diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/ErrorStatusHandlerLastAttemptIT.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/ErrorStatusHandlerLastAttemptIT.java
@@ -1,13 +1,13 @@
 package io.javaoperatorsdk.operator;
 
+import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 import io.fabric8.kubernetes.api.model.ObjectMetaBuilder;
 import io.javaoperatorsdk.operator.junit.LocallyRunOperatorExtension;
-import io.javaoperatorsdk.operator.processing.retry.GenericRetry;
 import io.javaoperatorsdk.operator.sample.errorstatushandler.ErrorStatusHandlerTestCustomResource;
 import io.javaoperatorsdk.operator.sample.errorstatushandler.ErrorStatusHandlerTestReconciler;
 
@@ -22,14 +22,13 @@ class ErrorStatusHandlerLastAttemptIT {
   @RegisterExtension
   LocallyRunOperatorExtension operator =
       LocallyRunOperatorExtension.builder()
-          .withReconciler(reconciler,
-              new GenericRetry().setMaxAttempts(MAX_RETRY_ATTEMPTS).withLinearRetry())
+          .withReconciler(reconciler, Optional.empty())
           .build();
 
   @Test
   void testErrorMessageSetEventually() {
     ErrorStatusHandlerTestCustomResource resource =
-        operator.create(ErrorStatusHandlerTestCustomResource.class, createCustomResource());
+        operator.create(createCustomResource());
 
     await()
         .atMost(10, TimeUnit.SECONDS)
diff --git a/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/AnnotationControllerConfigurationTest.java b/operator-framework/src/test/java/io/javaoperatorsdk/operator/config/runtime/AnnotationControllerConfigurationTest.java
