refinements based upon review feedback

moving handling of known resource versions, and making the config names
more descriptive

Signed-off-by: Steve Hawkins <[email protected]>

Author: shawkins

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationService.java

@@ -365,20 +365,20 @@ default Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
   }
 
   /**
-   * If an annotation should be used so that the operator sdk can detect events from its own updates
-   * of dependent resources and then filter them.
+   * If a javaoperatorsdk.io/previous annotation should be used so that the operator sdk can detect
+   * events from its own updates of dependent resources and then filter them.
    * <p>
    * Disable this if you want to react to your own dependent resource updates
    *
    * @since 4.5.0
    */
-  default boolean previousAnnotationForDependentResources() {
+  default boolean previousAnnotationForDependentResourcesEventFiltering() {
     return true;
   }
 
   /**
-   * If the event logic should parse the resourceVersion to determine the ordering of events. This
-   * is typically not needed.
+   * If the event logic should parse the resourceVersion to determine the ordering of dependent
+   * resource events. This is typically not needed.
    * <p>
    * Disabled by default as Kubernetes does not support, and discourages, this interpretation of
    * resourceVersions. Enable only if your api server event processing seems to lag the operator
@@ -387,7 +387,7 @@ default boolean previousAnnotationForDependentResources() {
    *
    * @since 4.5.0
    */
-  default boolean parseResourceVersions() {
+  default boolean parseResourceVersionsForEventFilteringAndCaching() {
     return false;
   }
 

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/ConfigurationServiceOverrider.java

@@ -286,17 +286,17 @@ public Set<Class<? extends HasMetadata>> defaultNonSSAResource() {
       }
 
       @Override
-      public boolean previousAnnotationForDependentResources() {
+      public boolean previousAnnotationForDependentResourcesEventFiltering() {
         return previousAnnotationForDependentResources != null
             ? previousAnnotationForDependentResources
-            : super.previousAnnotationForDependentResources();
+            : super.previousAnnotationForDependentResourcesEventFiltering();
       }
 
       @Override
-      public boolean parseResourceVersions() {
+      public boolean parseResourceVersionsForEventFilteringAndCaching() {
         return parseResourceVersions != null
             ? parseResourceVersions
-            : super.parseResourceVersions();
+            : super.parseResourceVersionsForEventFilteringAndCaching();
       }
     };
   }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java

@@ -211,7 +211,7 @@ protected boolean useSSA(Context<P> context) {
 
   private boolean usePreviousAnnotation(Context<P> context) {
     return context.getControllerConfiguration().getConfigurationService()
-        .previousAnnotationForDependentResources();
+        .previousAnnotationForDependentResourcesEventFiltering();
   }
 
   @Override

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/InformerEventSource.java

@@ -69,8 +69,6 @@ public class InformerEventSource<R extends HasMetadata, P extends HasMetadata>
     extends ManagedInformerEventSource<R, P, InformerConfiguration<R>>
     implements ResourceEventHandler<R> {
 
-  private static final int MAX_RESOURCE_VERSIONS = 256;
-
   public static String PREVIOUS_ANNOTATION_KEY = "javaoperatorsdk.io/previous";
 
   private static final Logger log = LoggerFactory.getLogger(InformerEventSource.class);
@@ -80,12 +78,12 @@ public class InformerEventSource<R extends HasMetadata, P extends HasMetadata>
   private final PrimaryToSecondaryMapper<P> primaryToSecondaryMapper;
   private Map<String, Function<R, List<String>>> indexerBuffer = new HashMap<>();
   private final String id = UUID.randomUUID().toString();
-  private final Set<String> knownResourceVersions;
 
   public InformerEventSource(
       InformerConfiguration<R> configuration, EventSourceContext<P> context) {
     this(configuration, context.getClient(),
-        context.getControllerConfiguration().getConfigurationService().parseResourceVersions());
+        context.getControllerConfiguration().getConfigurationService()
+            .parseResourceVersionsForEventFilteringAndCaching());
   }
 
   public InformerEventSource(InformerConfiguration<R> configuration, KubernetesClient client) {
@@ -95,17 +93,6 @@ public InformerEventSource(InformerConfiguration<R> configuration, KubernetesCli
   public InformerEventSource(InformerConfiguration<R> configuration, KubernetesClient client,
       boolean parseResourceVersions) {
     super(client.resources(configuration.getResourceClass()), configuration, parseResourceVersions);
-    if (parseResourceVersions) {
-      knownResourceVersions = Collections.newSetFromMap(new LinkedHashMap<>() {
-        @Override
-        protected boolean removeEldestEntry(java.util.Map.Entry<String, Boolean> eldest) {
-          return size() >= MAX_RESOURCE_VERSIONS;
-        }
-      });
-    } else {
-      knownResourceVersions = null;
-    }
-
     // If there is a primary to secondary mapper there is no need for primary to secondary index.
     primaryToSecondaryMapper = configuration.getPrimaryToSecondaryMapper();
     if (primaryToSecondaryMapper == null) {
@@ -188,8 +175,7 @@ private synchronized void onAddOrUpdate(Operation operation, R newObject, R oldO
   }
 
   private boolean canSkipEvent(R newObject, R oldObject, ResourceID resourceID) {
-    if (knownResourceVersions != null
-        && knownResourceVersions.contains(newObject.getMetadata().getResourceVersion())) {
+    if (temporaryResourceCache.isKnownResourceVersion(newObject)) {
       return true;
     }
     var res = temporaryResourceCache.getResourceFromCache(resourceID);
@@ -285,9 +271,6 @@ public synchronized void handleRecentResourceCreate(ResourceID resourceID, R res
 
   private void handleRecentCreateOrUpdate(Operation operation, R newResource, R oldResource) {
     primaryToSecondaryIndex.onAddOrUpdate(newResource);
-    if (knownResourceVersions != null) {
-      knownResourceVersions.add(newResource.getMetadata().getResourceVersion());
-    }
     temporaryResourceCache.putResource(newResource, Optional.ofNullable(oldResource)
         .map(r -> r.getMetadata().getResourceVersion()).orElse(null));
   }

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCache.java

@@ -1,7 +1,10 @@
 package io.javaoperatorsdk.operator.processing.event.source.informer;
 
+import java.util.Collections;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import java.util.Optional;
+import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 
 import org.slf4j.Logger;
@@ -34,15 +37,27 @@
 public class TemporaryResourceCache<T extends HasMetadata> {
 
   private static final Logger log = LoggerFactory.getLogger(TemporaryResourceCache.class);
+  private static final int MAX_RESOURCE_VERSIONS = 256;
 
   private final Map<ResourceID, T> cache = new ConcurrentHashMap<>();
   private final ManagedInformerEventSource<T, ?, ?> managedInformerEventSource;
   private final boolean parseResourceVersions;
+  private final Set<String> knownResourceVersions;
 
   public TemporaryResourceCache(ManagedInformerEventSource<T, ?, ?> managedInformerEventSource,
       boolean parseResourceVersions) {
     this.managedInformerEventSource = managedInformerEventSource;
     this.parseResourceVersions = parseResourceVersions;
+    if (parseResourceVersions) {
+      knownResourceVersions = Collections.newSetFromMap(new LinkedHashMap<String, Boolean>() {
+        @Override
+        protected boolean removeEldestEntry(java.util.Map.Entry<String, Boolean> eldest) {
+          return size() >= MAX_RESOURCE_VERSIONS;
+        }
+      });
+    } else {
+      knownResourceVersions = null;
+    }
   }
 
   public synchronized void onEvent(T resource, boolean unknownState) {
@@ -62,6 +77,9 @@ public synchronized void putAddedResource(T newResource) {
    * @param previousResourceVersion null indicates an add
    */
   public synchronized void putResource(T newResource, String previousResourceVersion) {
+    if (knownResourceVersions != null) {
+      knownResourceVersions.add(newResource.getMetadata().getResourceVersion());
+    }
     var resourceId = ResourceID.fromResource(newResource);
     var cachedResource = getResourceFromCache(resourceId)
         .orElse(managedInformerEventSource.get(resourceId).orElse(null));
@@ -79,6 +97,11 @@ public synchronized void putResource(T newResource, String previousResourceVersi
     }
   }
 
+  public boolean isKnownResourceVersion(T resource) {
+    return knownResourceVersions != null
+        && knownResourceVersions.contains(resource.getMetadata().getResourceVersion());
+  }
+
   /**
    * @return true if {@link InformerConfiguration#parseResourceVersions()} is enabled and the
    *         resourceVersion of newResource is numerically greater than cachedResource, otherwise

operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/source/informer/TemporaryResourceCacheTest.java

@@ -91,12 +91,15 @@ void removesResourceFromCache() {
   void resourceVersionParsing() {
     this.temporaryResourceCache = new TemporaryResourceCache<>(informerEventSource, true);
 
+    assertThat(temporaryResourceCache.isKnownResourceVersion(testResource())).isFalse();
+
     ConfigMap testResource = propagateTestResourceToCache();
 
     // an event with a newer version will not remove
-    temporaryResourceCache.onEvent(new ConfigMapBuilder(testResource()).editMetadata()
+    temporaryResourceCache.onEvent(new ConfigMapBuilder(testResource).editMetadata()
         .withResourceVersion("1").endMetadata().build(), false);
 
+    assertThat(temporaryResourceCache.isKnownResourceVersion(testResource)).isTrue();
     assertThat(temporaryResourceCache.getResourceFromCache(ResourceID.fromResource(testResource)))
         .isPresent();
 

operator-framework/src/test/java/io/javaoperatorsdk/operator/CreateUpdateInformerEventSourceEventFilterIT.java

@@ -31,19 +31,19 @@ void updateEventNotReceivedAfterCreateOrUpdate() {
     var createdResource =
         operator.create(resource);
 
-    assertData(operator, createdResource, 1);
+    assertData(operator, createdResource, 1, 1);
 
     CreateUpdateEventFilterTestCustomResource actualCreatedResource =
         operator.get(CreateUpdateEventFilterTestCustomResource.class,
             resource.getMetadata().getName());
     actualCreatedResource.getSpec().setValue("2");
     operator.replace(actualCreatedResource);
 
-    assertData(operator, actualCreatedResource, 2);
+    assertData(operator, actualCreatedResource, 2, 2);
   }
 
   static void assertData(LocallyRunOperatorExtension operator,
-      CreateUpdateEventFilterTestCustomResource resource, int executions) {
+      CreateUpdateEventFilterTestCustomResource resource, int minExecutions, int maxExecutions) {
     await()
         .atMost(Duration.ofSeconds(1))
         .until(() -> {
@@ -56,10 +56,10 @@ static void assertData(LocallyRunOperatorExtension operator,
               .equals(resource.getSpec().getValue());
         });
 
-    assertThat(
-        ((CreateUpdateEventFilterTestReconciler) operator.getFirstReconciler())
-            .getNumberOfExecutions())
-        .isEqualTo(executions);
+    int numberOfExecutions = ((CreateUpdateEventFilterTestReconciler) operator.getFirstReconciler())
+        .getNumberOfExecutions();
+    assertThat(numberOfExecutions).isGreaterThanOrEqualTo(minExecutions);
+    assertThat(numberOfExecutions).isLessThanOrEqualTo(maxExecutions);
   }
 
   static CreateUpdateEventFilterTestCustomResource prepareTestResource() {

operator-framework/src/test/java/io/javaoperatorsdk/operator/PreviousAnnotationDisabledIT.java

@@ -24,15 +24,15 @@ void updateEventReceivedAfterCreateOrUpdate() {
     var createdResource =
         operator.create(resource);
 
-    CreateUpdateInformerEventSourceEventFilterIT.assertData(operator, createdResource, 2);
+    CreateUpdateInformerEventSourceEventFilterIT.assertData(operator, createdResource, 1, 2);
 
     CreateUpdateEventFilterTestCustomResource actualCreatedResource =
         operator.get(CreateUpdateEventFilterTestCustomResource.class,
             resource.getMetadata().getName());
     actualCreatedResource.getSpec().setValue("2");
     operator.replace(actualCreatedResource);
 
-    CreateUpdateInformerEventSourceEventFilterIT.assertData(operator, actualCreatedResource, 4);
+    CreateUpdateInformerEventSourceEventFilterIT.assertData(operator, actualCreatedResource, 2, 4);
   }
 
 }