simplifying

sergiitk · Dec 29, 2021 · 163e103 · 163e103
1 parent 33d7f54
commit 163e103
Show file tree

Hide file tree

Showing 5 changed files with 63 additions and 43 deletions.
diff --git a/xds/src/main/java/io/grpc/xds/ClientXdsClient.java b/xds/src/main/java/io/grpc/xds/ClientXdsClient.java
@@ -118,7 +118,6 @@
 import java.util.Objects;
 import java.util.Set;
 import java.util.concurrent.ExecutionException;
-import java.util.concurrent.Future;
 import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.TimeoutException;
@@ -1972,7 +1971,37 @@ public void run() {
         future.set(getSubscribedResourcesMetadataUnsafe(type));
       }
     });
-    return awaitSubscribedResourcesMetadata(future);
+    try {
+      return future.get(METADATA_SYNC_TIMEOUT_SEC, TimeUnit.SECONDS);
+    } catch (InterruptedException | ExecutionException | TimeoutException e) {
+      throw new MetadataLoadException(e);
+    }
+  }
+
+  @Override
+  Map<ResourceType, Map<String, ResourceMetadata>> getSubscribedResourcesMetadataSnapshot() {
+    final SettableFuture<Map<ResourceType, Map<String, ResourceMetadata>>> future =
+        SettableFuture.create();
+    syncContext.execute(new Runnable() {
+      @Override
+      public void run() {
+        // A map from ResourceType to a map (ResourceName: ResourceMetadata)
+        ImmutableMap.Builder<ResourceType, Map<String, ResourceMetadata>> metadataSnapshot =
+            ImmutableMap.builder();
+        for (ResourceType type : ResourceType.values()) {
+          if (type == ResourceType.UNKNOWN) {
+            continue;
+          }
+          metadataSnapshot.put(type, getSubscribedResourcesMetadataUnsafe(type));
+        }
+        future.set(metadataSnapshot.build());
+      }
+    });
+    try {
+      return future.get(METADATA_SYNC_TIMEOUT_SEC, TimeUnit.SECONDS);
+    } catch (InterruptedException | ExecutionException | TimeoutException e) {
+      throw new MetadataLoadException(e);
+    }
   }
 
   private Map<String, ResourceMetadata> getSubscribedResourcesMetadataUnsafe(ResourceType type) {
@@ -1983,19 +2012,6 @@ private Map<String, ResourceMetadata> getSubscribedResourcesMetadataUnsafe(Resou
     return metadataMap.build();
   }
 
-  private static Map<String, ResourceMetadata> awaitSubscribedResourcesMetadata(
-      Future<Map<String, ResourceMetadata>> future) {
-    try {
-      return future.get(METADATA_SYNC_TIMEOUT_SEC, TimeUnit.SECONDS);
-    } catch (InterruptedException e) {
-      throw new MetadataLoadException(e);
-    } catch (ExecutionException e) {
-      throw new MetadataLoadException(e);
-    } catch (TimeoutException e) {
-      throw new MetadataLoadException(e);
-    }
-  }
-
   @Override
   TlsContextManager getTlsContextManager() {
     return tlsContextManager;

diff --git a/xds/src/main/java/io/grpc/xds/CsdsService.java b/xds/src/main/java/io/grpc/xds/CsdsService.java
@@ -143,16 +143,15 @@ private ClientStatusResponse getConfigDumpForRequest(ClientStatusRequest request
   static ClientConfig getClientConfigForXdsClient(XdsClient xdsClient) {
     ClientConfig.Builder builder = ClientConfig.newBuilder()
         .setNode(xdsClient.getBootstrapInfo().node().toEnvoyProtoNode());
-    for (ResourceType type : ResourceType.values()) {
-      if (type == ResourceType.UNKNOWN) {
-        continue;
-      }
-      Map<String, ResourceMetadata> metadataMap = xdsClient.getSubscribedResourcesMetadata(type);
-      for (String resourceName : metadataMap.keySet()) {
-        ResourceMetadata metadata = metadataMap.get(resourceName);
+    for (Map.Entry<ResourceType, Map<String, ResourceMetadata>> metadataSnapshotEntry
+        : xdsClient.getSubscribedResourcesMetadataSnapshot().entrySet()) {
+      ResourceType type = metadataSnapshotEntry.getKey();
+      for (Map.Entry<String, ResourceMetadata> metadataEntry
+          : metadataSnapshotEntry.getValue().entrySet()) {
+        ResourceMetadata metadata = metadataEntry.getValue();
         GenericXdsConfig.Builder genericXdsConfigBuilder = GenericXdsConfig.newBuilder()
             .setTypeUrl(type.typeUrl())
-            .setName(resourceName)
+            .setName(metadataEntry.getKey())
             .setClientStatus(metadataStatusToClientStatus(metadata.getStatus()));
         if (metadata.getRawResource() != null) {
           genericXdsConfigBuilder
@@ -161,6 +160,7 @@ static ClientConfig getClientConfigForXdsClient(XdsClient xdsClient) {
               .setXdsConfig(metadata.getRawResource());
         }
         if (metadata.getStatus() == ResourceMetadataStatus.NACKED) {
+          assert metadata.getErrorState() != null;
           genericXdsConfigBuilder
               .setErrorState(metadataUpdateFailureStateToProto(metadata.getErrorState()));
         }

diff --git a/xds/src/main/java/io/grpc/xds/XdsClient.java b/xds/src/main/java/io/grpc/xds/XdsClient.java
@@ -498,6 +498,15 @@ Map<String, ResourceMetadata> getSubscribedResourcesMetadata(ResourceType type)
     throw new UnsupportedOperationException();
   }
 
+  /**
+   * Returns the map containing the {@link ResourceMetadata} of the subscribed resources for the
+   * given resource type, indexed by the resource name.
+   */
+  // Must be synchronized.
+  Map<ResourceType, Map<String, ResourceMetadata>> getSubscribedResourcesMetadataSnapshot() {
+    throw new UnsupportedOperationException();
+  }
+
   /**
    * Registers a data watcher for the given LDS resource.
    */

diff --git a/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java b/xds/src/test/java/io/grpc/xds/ClientXdsClientTestBase.java
@@ -379,10 +379,12 @@ protected static boolean matchErrorDetail(
 
   private void verifySubscribedResourcesMetadataSizes(
       int ldsSize, int cdsSize, int rdsSize, int edsSize) {
-    assertThat(xdsClient.getSubscribedResourcesMetadata(LDS)).hasSize(ldsSize);
-    assertThat(xdsClient.getSubscribedResourcesMetadata(CDS)).hasSize(cdsSize);
-    assertThat(xdsClient.getSubscribedResourcesMetadata(RDS)).hasSize(rdsSize);
-    assertThat(xdsClient.getSubscribedResourcesMetadata(EDS)).hasSize(edsSize);
+    Map<ResourceType, Map<String, ResourceMetadata>> subscribedResourcesMetadata =
+        xdsClient.getSubscribedResourcesMetadataSnapshot();
+    assertThat(subscribedResourcesMetadata.get(LDS)).hasSize(ldsSize);
+    assertThat(subscribedResourcesMetadata.get(CDS)).hasSize(cdsSize);
+    assertThat(subscribedResourcesMetadata.get(RDS)).hasSize(rdsSize);
+    assertThat(subscribedResourcesMetadata.get(EDS)).hasSize(edsSize);
   }
 
   /** Verify the resource requested, but not updated. */
@@ -435,7 +437,7 @@ private ResourceMetadata verifyResourceMetadata(
       ResourceType type, String resourceName, Any rawResource, ResourceMetadataStatus status,
       String versionInfo, long updateTimeNanos, boolean hasErrorState) {
     ResourceMetadata resourceMetadata =
-        xdsClient.getSubscribedResourcesMetadata(type).get(resourceName);
+        xdsClient.getSubscribedResourcesMetadataSnapshot().get(type).get(resourceName);
     assertThat(resourceMetadata).isNotNull();
     String name = type.toString() + " resource '" + resourceName + "' metadata field ";
     assertWithMessage(name + "status").that(resourceMetadata.getStatus()).isEqualTo(status);

diff --git a/xds/src/test/java/io/grpc/xds/CsdsServiceTest.java b/xds/src/test/java/io/grpc/xds/CsdsServiceTest.java
@@ -81,7 +81,7 @@ Bootstrapper.BootstrapInfo getBootstrapInfo() {
     }
 
     @Override
-    Map<String, ResourceMetadata> getSubscribedResourcesMetadata(ResourceType type) {
+    Map<ResourceType, Map<String, ResourceMetadata>> getSubscribedResourcesMetadataSnapshot() {
       return ImmutableMap.of();
     }
   };
@@ -302,20 +302,13 @@ Bootstrapper.BootstrapInfo getBootstrapInfo() {
         }
 
         @Override
-        Map<String, ResourceMetadata> getSubscribedResourcesMetadata(ResourceType type) {
-          switch (type) {
-            case LDS:
-              return ImmutableMap.of("subscribedResourceName." + type.name(), METADATA_ACKED_LDS);
-            case RDS:
-              return ImmutableMap.of("subscribedResourceName." + type.name(), METADATA_ACKED_RDS);
-            case CDS:
-              return ImmutableMap.of("subscribedResourceName." + type.name(), METADATA_ACKED_CDS);
-            case EDS:
-              return ImmutableMap.of("subscribedResourceName." + type.name(), METADATA_ACKED_EDS);
-            case UNKNOWN:
-            default:
-              throw new AssertionError("Unexpected resource name");
-          }
+        Map<ResourceType, Map<String, ResourceMetadata>> getSubscribedResourcesMetadataSnapshot() {
+          return new ImmutableMap.Builder<ResourceType, Map<String, ResourceMetadata>>()
+            .put(LDS, ImmutableMap.of("subscribedResourceName.LDS", METADATA_ACKED_LDS))
+            .put(RDS, ImmutableMap.of("subscribedResourceName.RDS", METADATA_ACKED_RDS))
+            .put(CDS, ImmutableMap.of("subscribedResourceName.CDS", METADATA_ACKED_CDS))
+            .put(EDS, ImmutableMap.of("subscribedResourceName.EDS", METADATA_ACKED_EDS))
+            .build();
         }
       });