Dont save whole response just data needed during another request (#133)

* Dont save whole response just data needed during another request

Signed-off-by: Lukasz Dziedziak <[email protected]>

* Changed to AutoValue, added small comment, changed property name

Signed-off-by: Lukasz Dziedziak <[email protected]>

* Changed why how we take resourceNames

Signed-off-by: Lukasz Dziedziak <[email protected]>

Co-authored-by: Lukasz Dziedziak <[email protected]>

Author: lukidzi

server/pom.xml

@@ -17,6 +17,13 @@
             <version>0.1.23-SNAPSHOT</version>
         </dependency>
 
+        <dependency>
+            <groupId>com.google.auto.value</groupId>
+            <artifactId>auto-value</artifactId>
+            <version>${auto-value.version}</version>
+            <scope>provided</scope>
+        </dependency>
+
         <dependency>
             <groupId>io.grpc</groupId>
             <artifactId>grpc-netty</artifactId>

server/src/main/java/io/envoyproxy/controlplane/server/AdsDiscoveryRequestStreamObserver.java

@@ -21,7 +21,7 @@
  */
 public class AdsDiscoveryRequestStreamObserver extends DiscoveryRequestStreamObserver {
   private final ConcurrentMap<String, Watch> watches;
-  private final ConcurrentMap<String, DiscoveryResponse> latestResponse;
+  private final ConcurrentMap<String, LatestDiscoveryResponse> latestResponse;
   private final ConcurrentMap<String, Set<String>> ackedResources;
 
   AdsDiscoveryRequestStreamObserver(StreamObserver<DiscoveryResponse> responseObserver,
@@ -59,12 +59,12 @@ boolean ads() {
   }
 
   @Override
-  DiscoveryResponse latestResponse(String typeUrl) {
+  LatestDiscoveryResponse latestResponse(String typeUrl) {
     return latestResponse.get(typeUrl);
   }
 
   @Override
-  void setLatestResponse(String typeUrl, DiscoveryResponse response) {
+  void setLatestResponse(String typeUrl, LatestDiscoveryResponse response) {
     latestResponse.put(typeUrl, response);
   }
 

server/src/main/java/io/envoyproxy/controlplane/server/DiscoveryRequestStreamObserver.java

@@ -71,16 +71,12 @@ public void onNext(DiscoveryRequest request) {
       return;
     }
 
-    DiscoveryResponse latestResponse = latestResponse(requestTypeUrl);
-    String resourceNonce = latestResponse == null ? null : latestResponse.getNonce();
+    LatestDiscoveryResponse latestDiscoveryResponse = latestResponse(requestTypeUrl);
+    String resourceNonce = latestDiscoveryResponse == null ? null : latestDiscoveryResponse.nonce();
 
     if (isNullOrEmpty(resourceNonce) || resourceNonce.equals(nonce)) {
-      if (!request.hasErrorDetail() && latestResponse != null) {
-        Set<String> ackedResourcesForType = latestResponse.getResourcesList()
-            .stream()
-            .map(Resources::getResourceName)
-            .collect(Collectors.toSet());
-        setAckedResources(requestTypeUrl, ackedResourcesForType);
+      if (!request.hasErrorDetail() && latestDiscoveryResponse != null) {
+        setAckedResources(requestTypeUrl, latestDiscoveryResponse.resourceNames());
       }
 
       computeWatch(requestTypeUrl, () -> discoverySever.configWatcher.createWatch(
@@ -155,7 +151,13 @@ private void send(Response response, String typeUrl) {
     // Store the latest response *before* we send the response. This ensures that by the time the request
     // is processed the map is guaranteed to be updated. Doing it afterwards leads to a race conditions
     // which may see the incoming request arrive before the map is updated, failing the nonce check erroneously.
-    setLatestResponse(typeUrl, discoveryResponse);
+    setLatestResponse(
+        typeUrl,
+        LatestDiscoveryResponse.create(
+            nonce,
+            response.resources().stream().map(Resources::getResourceName).collect(Collectors.toSet())
+        )
+    );
     synchronized (responseObserver) {
       if (!isClosing) {
         try {
@@ -173,9 +175,9 @@ private void send(Response response, String typeUrl) {
 
   abstract boolean ads();
 
-  abstract DiscoveryResponse latestResponse(String typeUrl);
+  abstract LatestDiscoveryResponse latestResponse(String typeUrl);
 
-  abstract void setLatestResponse(String typeUrl, DiscoveryResponse response);
+  abstract void setLatestResponse(String typeUrl, LatestDiscoveryResponse response);
 
   abstract Set<String> ackedResources(String typeUrl);
 

server/src/main/java/io/envoyproxy/controlplane/server/LatestDiscoveryResponse.java

@@ -0,0 +1,18 @@
+package io.envoyproxy.controlplane.server;
+
+import com.google.auto.value.AutoValue;
+import java.util.Set;
+
+/**
+ * Class introduces optimization which store only required data during next request.
+ */
+@AutoValue
+public abstract class LatestDiscoveryResponse {
+  static LatestDiscoveryResponse create(String nonce, Set<String> resourceNames) {
+    return new AutoValue_LatestDiscoveryResponse(nonce, resourceNames);
+  }
+
+  abstract String nonce();
+
+  abstract Set<String> resourceNames();
+}

server/src/main/java/io/envoyproxy/controlplane/server/XdsDiscoveryRequestStreamObserver.java

@@ -14,7 +14,7 @@
  */
 public class XdsDiscoveryRequestStreamObserver extends DiscoveryRequestStreamObserver {
   private volatile Watch watch;
-  private volatile DiscoveryResponse latestResponse;
+  private volatile LatestDiscoveryResponse latestDiscoveryResponse;
   // ackedResources is only used in the same thread so it need not be volatile
   private Set<String> ackedResources;
 
@@ -40,13 +40,13 @@ boolean ads() {
   }
 
   @Override
-  DiscoveryResponse latestResponse(String typeUrl) {
-    return latestResponse;
+  LatestDiscoveryResponse latestResponse(String typeUrl) {
+    return latestDiscoveryResponse;
   }
 
   @Override
-  void setLatestResponse(String typeUrl, DiscoveryResponse response) {
-    latestResponse = response;
+  void setLatestResponse(String typeUrl, LatestDiscoveryResponse response) {
+    latestDiscoveryResponse = response;
   }
 
   @Override