fix fabric8io#4931: small performance refinements to crud mock

shawkins · Mar 6, 2023 · b322b47 · b322b47
1 parent 85ce40f
commit b322b47
Show file tree

Hide file tree

Showing 5 changed files with 40 additions and 32 deletions.
diff --git a/...mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesCrudDispatcher.java b/...mock/src/main/java/io/fabric8/kubernetes/client/server/mock/KubernetesCrudDispatcher.java
@@ -48,6 +48,8 @@
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArraySet;
 import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.ReadWriteLock;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
 
 public class KubernetesCrudDispatcher extends CrudDispatcher implements KubernetesCrudPersistence, CustomResourceAware {
 
@@ -60,6 +62,7 @@ public class KubernetesCrudDispatcher extends CrudDispatcher implements Kubernet
   private final KubernetesCrudDispatcherHandler postHandler;
   private final KubernetesCrudDispatcherHandler putHandler;
   private final KubernetesCrudDispatcherHandler patchHandler;
+  private final ReadWriteLock lock = new ReentrantReadWriteLock();
 
   public KubernetesCrudDispatcher() {
     this(Collections.emptyList());
@@ -81,12 +84,13 @@ public KubernetesCrudDispatcher(List<CustomResourceDefinitionContext> crdContext
   }
 
   MockResponse process(RecordedRequest request, KubernetesCrudDispatcherHandler handler) {
-    synchronized (map) {
-      try {
-        return handler.handle(request);
-      } catch (KubernetesCrudDispatcherException e) {
-        return new MockResponse().setResponseCode(e.getCode()).setBody(e.toStatusBody());
-      }
+    lock.writeLock().lock();
+    try {
+      return handler.handle(request);
+    } catch (KubernetesCrudDispatcherException e) {
+      return new MockResponse().setResponseCode(e.getCode()).setBody(e.toStatusBody());
+    } finally {
+      lock.writeLock().unlock();
     }
   }
 
@@ -120,11 +124,14 @@ public MockResponse handleUpdate(RecordedRequest request) {
    */
   @Override
   public MockResponse handleGet(String path) {
-    synchronized (map) {
+    lock.readLock().lock();
+    try {
       if (detectWatchMode(path)) {
         return handleWatch(path);
       }
       return handle(path, null);
+    } finally {
+      lock.readLock().unlock();
     }
   }
 
@@ -188,28 +195,26 @@ public MockResponse handlePatch(RecordedRequest request) {
    */
   @Override
   public MockResponse handleDelete(String path) {
-    synchronized (map) {
+    lock.writeLock().lock();
+    try {
       return handle(path, this::processDelete);
+    } finally {
+      lock.writeLock().unlock();
     }
   }
 
   private void processDelete(String path, AttributeSet pathAttributes, AttributeSet oldAttributes) {
     String jsonStringOfResource = map.get(oldAttributes);
-    /*
-     * Potential performance improvement: The resource is unmarshalled and marshalled in other places (e.g., when creating a
-     * WatchEvent later).
-     * This could be avoided by storing the unmarshalled object (instead of a String) in the map.
-     */
     final GenericKubernetesResource resource = Serialization.unmarshal(jsonStringOfResource, GenericKubernetesResource.class);
     if (resource.getFinalizers().isEmpty()) {
       // No finalizers left, actually remove the resource.
-      processEvent(path, pathAttributes, oldAttributes, null);
+      processEvent(path, pathAttributes, oldAttributes, null, null);
       return;
     } else if (!resource.isMarkedForDeletion()) {
       // Mark the resource as deleted, but don't remove it yet (wait for finalizer-removal).
       resource.getMetadata().setDeletionTimestamp(LocalDateTime.now().toString());
       String updatedResource = Serialization.asJson(resource);
-      processEvent(path, pathAttributes, oldAttributes, updatedResource);
+      processEvent(path, pathAttributes, oldAttributes, resource, updatedResource);
     }
     // else: if the resource is already marked for deletion and still has finalizers, do nothing.
   }
@@ -226,11 +231,9 @@ public AttributeSet getKey(String path) {
 
   @Override
   public Map.Entry<AttributeSet, String> findResource(AttributeSet attributes) {
-    synchronized (map) {
-      return map.entrySet().stream()
-          .filter(entry -> entry.getKey().matches(attributes))
-          .findFirst().orElse(null);
-    }
+    return map.entrySet().stream()
+        .filter(entry -> entry.getKey().matches(attributes))
+        .findFirst().orElse(null);
   }
 
   @Override
@@ -239,11 +242,16 @@ public boolean isStatusSubresourceEnabledForResource(String path) {
   }
 
   @Override
-  public void processEvent(String path, AttributeSet pathAttributes, AttributeSet oldAttributes, String newState) {
+  public void processEvent(String path, AttributeSet pathAttributes, AttributeSet oldAttributes,
+      GenericKubernetesResource resource, String newState) {
     String existing = map.remove(oldAttributes);
     AttributeSet newAttributes = null;
     if (newState != null) {
-      newAttributes = kubernetesAttributesExtractor.fromResource(newState);
+      if (resource != null) {
+        newAttributes = kubernetesAttributesExtractor.extract(resource);
+      } else {
+        newAttributes = kubernetesAttributesExtractor.fromResource(newState);
+      }
       // corner case - we need to get the plural from the path
       if (!newAttributes.containsKey(KubernetesAttributesExtractor.PLURAL)) {
         newAttributes = AttributeSet.merge(pathAttributes, newAttributes);
@@ -283,11 +291,9 @@ public MockResponse handleWatch(String path) {
     }
     WatchEventsListener watchEventListener = new WatchEventsListener(context, query, watchEventListeners, LOGGER,
         watch -> {
-          synchronized (map) {
-            map.entrySet().stream()
-                .filter(entry -> watch.attributeMatches(entry.getKey()))
-                .forEach(entry -> watch.sendWebSocketResponse(entry.getValue(), Action.ADDED));
-          }
+          map.entrySet().stream()
+              .filter(entry -> watch.attributeMatches(entry.getKey()))
+              .forEach(entry -> watch.sendWebSocketResponse(entry.getValue(), Action.ADDED));
         });
     watchEventListeners.add(watchEventListener);
     mockResponse.setSocketPolicy(SocketPolicy.KEEP_OPEN);

diff --git a/...rc/main/java/io/fabric8/kubernetes/client/server/mock/crud/KubernetesCrudPersistence.java b/...rc/main/java/io/fabric8/kubernetes/client/server/mock/crud/KubernetesCrudPersistence.java
@@ -19,6 +19,7 @@
 import com.fasterxml.jackson.databind.JsonNode;
 import com.fasterxml.jackson.databind.ObjectReader;
 import com.fasterxml.jackson.databind.node.ObjectNode;
+import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
 import io.fabric8.kubernetes.client.server.mock.Resetable;
 import io.fabric8.kubernetes.client.utils.Serialization;
 import io.fabric8.mockwebserver.crud.AttributeSet;
@@ -47,7 +48,7 @@ public interface KubernetesCrudPersistence extends Resetable {
 
   boolean isStatusSubresourceEnabledForResource(String path);
 
-  void processEvent(String path, AttributeSet pathAttributes, AttributeSet oldAttributes, String newState);
+  void processEvent(String path, AttributeSet pathAttributes, AttributeSet oldAttributes, GenericKubernetesResource resource, String newState);
 
   default JsonNode asNode(Map.Entry<AttributeSet, String> resource) throws KubernetesCrudDispatcherException {
     return asNode(resource.getValue());

diff --git a/...server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/crud/PatchHandler.java b/...server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/crud/PatchHandler.java
@@ -89,9 +89,10 @@ public MockResponse handle(String path, String contentType, String requestBody)
     if (deserializedResource.isMarkedForDeletion() && deserializedResource.getFinalizers().isEmpty()) {
       // Delete the resource.
       updatedAsString = null;
+      deserializedResource = null;
     }
 
-    persistence.processEvent(path, query, currentResourceEntry.getKey(), updatedAsString);
+    persistence.processEvent(path, query, currentResourceEntry.getKey(), deserializedResource, updatedAsString);
     return new MockResponse().setResponseCode(HTTP_ACCEPTED).setBody(Utils.getNonNullOrElse(updatedAsString, ""));
   }
 

diff --git a/...-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/crud/PostHandler.java b/...-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/crud/PostHandler.java
@@ -62,7 +62,7 @@ public MockResponse handle(String path, String contentType, String requestBody)
       resource.getAdditionalProperties().remove(STATUS);
     }
     final String response = Serialization.asJson(resource);
-    persistence.processEvent(path, attributes, null, response);
+    persistence.processEvent(path, attributes, null, resource, response);
     return new MockResponse().setResponseCode(HttpURLConnection.HTTP_CREATED).setBody(response);
   }
 

diff --git a/...s-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/crud/PutHandler.java b/...s-server-mock/src/main/java/io/fabric8/kubernetes/client/server/mock/crud/PutHandler.java
@@ -47,7 +47,7 @@ public MockResponse handle(String path, String contentType, String requestBody)
 
     // Delete the resource if it is marked for deletion and has no finalizers.
     if (resource.isMarkedForDeletion() && resource.getFinalizers().isEmpty()) {
-      persistence.processEvent(path, attributes, currentResourceEntry.getKey(), null);
+      persistence.processEvent(path, attributes, currentResourceEntry.getKey(), null, null);
       return new MockResponse().setResponseCode(HttpURLConnection.HTTP_OK);
     }
 
@@ -72,7 +72,7 @@ public MockResponse handle(String path, String contentType, String requestBody)
     }
     persistence.touchResourceVersion(currentResource, updatedResource);
     final String response = Serialization.asJson(updatedResource);
-    persistence.processEvent(path, attributes, currentResourceEntry.getKey(), response);
+    persistence.processEvent(path, attributes, currentResourceEntry.getKey(), null, response);
     return new MockResponse().setResponseCode(HttpURLConnection.HTTP_OK).setBody(response);
   }
 }