KNOX-2978 - Race condition between Service Discovery and Polling Conf…

…ig Analyzer
apache · Oct 26, 2023 · b956c75 · b956c75
1 parent c49302a
commit b956c75
Show file tree

Hide file tree

Showing 3 changed files with 6 additions and 19 deletions.
diff --git a/...n/java/org/apache/knox/gateway/topology/discovery/cm/ClouderaManagerServiceDiscovery.java b/...n/java/org/apache/knox/gateway/topology/discovery/cm/ClouderaManagerServiceDiscovery.java
@@ -253,8 +253,6 @@ private ClouderaManagerCluster discoverCluster(DiscoveryApiClient client, String
 
     log.discoveringCluster(clusterName);
 
-    repository.registerCluster(client.getConfig());
-
     Set<ServiceModel> serviceModels = new HashSet<>();
 
     List<ApiService> serviceList = getClusterServices(client.getConfig(), servicesResourceApi);
@@ -340,7 +338,7 @@ private boolean shouldSkipServiceDiscovery(List<ServiceModelGenerator> modelGene
   private List<ApiService> getClusterServices(ServiceDiscoveryConfig serviceDiscoveryConfig, ServicesResourceApi servicesResourceApi) throws ApiException {
     log.lookupClusterServicesFromRepository();
     List<ApiService> services = repository.getServices(serviceDiscoveryConfig);
-    if (services == null || services.isEmpty()) {
+    if (services.isEmpty()) {
       try {
         log.lookupClusterServicesFromCM();
         final ApiServiceList serviceList = servicesResourceApi.readServices(serviceDiscoveryConfig.getCluster(), VIEW_SUMMARY);

diff --git a/.../apache/knox/gateway/topology/discovery/cm/ClouderaManagerServiceDiscoveryRepository.java b/.../apache/knox/gateway/topology/discovery/cm/ClouderaManagerServiceDiscoveryRepository.java
@@ -59,23 +59,18 @@ void clear() {
     repository.clear();
   }
 
-  void registerCluster(ServiceDiscoveryConfig serviceDiscoveryConfig) {
-    repository.putIfAbsent(RepositoryKey.of(serviceDiscoveryConfig), Caffeine.newBuilder().expireAfterWrite(Duration.ofSeconds(cacheEntryTTL)).build());
-  }
-
   void addService(ServiceDiscoveryConfig serviceDiscoveryConfig, ApiService service) {
     getClusterServices(serviceDiscoveryConfig).put(service, new ServiceDetails());
   }
 
   List<ApiService> getServices(ServiceDiscoveryConfig serviceDiscoveryConfig) {
     final Cache<ApiService, ServiceDetails> clusterServices = getClusterServices(serviceDiscoveryConfig);
-    return clusterServices == null ? null
-        : clusterServices.asMap().entrySet().stream().filter(entry -> entry.getValue() != null).map(entry -> entry.getKey())
+    return clusterServices.asMap().entrySet().stream().filter(entry -> entry.getValue() != null).map(entry -> entry.getKey())
             .collect(Collectors.toList());
   }
 
   private Cache<ApiService, ServiceDetails> getClusterServices(ServiceDiscoveryConfig serviceDiscoveryConfig) {
-    return repository.get(RepositoryKey.of(serviceDiscoveryConfig));
+    return repository.computeIfAbsent(RepositoryKey.of(serviceDiscoveryConfig), (k) -> Caffeine.newBuilder().expireAfterWrite(Duration.ofSeconds(cacheEntryTTL)).build());
   }
 
   void addServiceConfig(ServiceDiscoveryConfig serviceDiscoveryConfig, ApiService service, ApiServiceConfig serviceConfig) {

diff --git a/...che/knox/gateway/topology/discovery/cm/ClouderaManagerServiceDiscoveryRepositoryTest.java b/...che/knox/gateway/topology/discovery/cm/ClouderaManagerServiceDiscoveryRepositoryTest.java
@@ -56,14 +56,13 @@ public static void init() {
   @Before
   public void setUp() {
     repository.clear();
-    assertNull(repository.getServices(serviceDiscoveryConfig));
     repository.setCacheEntryTTL(GatewayConfig.DEFAULT_CM_SERVICE_DISCOVERY_CACHE_ENTRY_TTL);
+    assertTrue(repository.getServices(serviceDiscoveryConfig).isEmpty());
   }
 
   @Test
   public void testRegisterCluster() throws Exception {
-    assertNull(repository.getServices(serviceDiscoveryConfig));
-    repository.registerCluster(serviceDiscoveryConfig);
+    assertTrue(repository.getServices(serviceDiscoveryConfig).isEmpty());
     final List<ApiService> services = repository.getServices(serviceDiscoveryConfig);
     assertNotNull(services);
     assertTrue(services.isEmpty());
@@ -72,7 +71,6 @@ public void testRegisterCluster() throws Exception {
   @Test
   public void testAddService() throws Exception {
     final String serviceName = "HDFS-1";
-    repository.registerCluster(serviceDiscoveryConfig);
     assertFalse(containsService(serviceName));
     final ApiService service = EasyMock.createNiceMock(ApiService.class);
     EasyMock.expect(service.getName()).andReturn(serviceName).anyTimes();
@@ -86,7 +84,6 @@ public void testAddServiceConfig() throws Exception {
     final String serviceName = "HDFS-1";
     final String serviceConfigName = "myServiceConfigName";
     final String serviceConfigValue = "myServiceConfigValue";
-    repository.registerCluster(serviceDiscoveryConfig);
     final ApiService service = EasyMock.createNiceMock(ApiService.class);
     EasyMock.expect(service.getName()).andReturn(serviceName).anyTimes();
     final ApiConfig serviceConfig = EasyMock.createNiceMock(ApiConfig.class);
@@ -105,7 +102,6 @@ public void testAddServiceConfig() throws Exception {
   public void testAddRole() throws Exception {
     final String serviceName = "HDFS-1";
     final String roleName = "NAMENODE-1";
-    repository.registerCluster(serviceDiscoveryConfig);
     final ApiService service = EasyMock.createNiceMock(ApiService.class);
     EasyMock.expect(service.getName()).andReturn(serviceName).anyTimes();
     final ApiRole role = EasyMock.createNiceMock(ApiRole.class);
@@ -121,7 +117,6 @@ public void testAddRole() throws Exception {
 
   @Test
   public void testAddNullRoles() throws Exception {
-    repository.registerCluster(serviceDiscoveryConfig);
     final ApiService service = EasyMock.createNiceMock(ApiService.class);
     EasyMock.expect(service.getName()).andReturn(ClouderaManagerServiceDiscovery.CORE_SETTINGS_TYPE).anyTimes();
     EasyMock.replay(service);
@@ -132,7 +127,6 @@ public void testAddNullRoles() throws Exception {
 
   @Test
   public void testAddNullRoleItems() throws Exception {
-    repository.registerCluster(serviceDiscoveryConfig);
     final ApiService service = EasyMock.createNiceMock(ApiService.class);
     EasyMock.expect(service.getName()).andReturn(ClouderaManagerServiceDiscovery.CORE_SETTINGS_TYPE).anyTimes();
     final ApiRoleList roles = EasyMock.createNiceMock(ApiRoleList.class);
@@ -150,7 +144,6 @@ public void testAddRoleConfig() throws Exception {
     final String roleConfigName = "myRoleConfig";
     final String roleConfigValue = "myRoleConfigValue";
 
-    repository.registerCluster(serviceDiscoveryConfig);
     final ApiService service = EasyMock.createNiceMock(ApiService.class);
     EasyMock.expect(service.getName()).andReturn(serviceName).anyTimes();
     final ApiRole role = EasyMock.createNiceMock(ApiRole.class);
@@ -174,6 +167,7 @@ public void testAddRoleConfig() throws Exception {
   public void testCacheAutoEviction() throws Exception {
     final long entryTTL = 1;
     repository.setCacheEntryTTL(entryTTL);
+    repository.clear();
     testAddService();
     TimeUnit.SECONDS.sleep(entryTTL + 1);
     assertFalse(containsService("HDFS-1"));