[GitHub] [geode] jinmeiliao commented on a diff in pull request #7703: GEODE-10318: do not add duplicate entries in the locators list

[GitBox <gi...@apache.org>]

jinmeiliao commented on code in PR #7703:
URL: https://github.com/apache/geode/pull/7703#discussion_r877573410


##########
geode-core/src/main/java/org/apache/geode/distributed/internal/InternalLocator.java:
##########
@@ -707,71 +711,102 @@ private void startDistributedSystem() throws IOException {
       // LOG: changed from config to info
       logger.info("Using existing distributed system: {}", existing);
       startCache(existing);
-    } else {
+      return;
+    }
 
-      StringBuilder sb = new StringBuilder(100);
-      if (hostAddress != null && !StringUtils.isEmpty(hostAddress.getHostName())) {
-        sb.append(hostAddress.getHostName());
-      } else {
-        sb.append(LocalHostUtil.getLocalHost().getCanonicalHostName());
-      }
-      sb.append('[').append(getPort()).append(']');
-      String thisLocator = sb.toString();
-
-      if (peerLocator) {
-        // append this locator to the locators list from the config properties
-        boolean setLocatorsProp = false;
-        String locatorsConfigValue = distributionConfig.getLocators();
-        if (StringUtils.isNotBlank(locatorsConfigValue)) {
-          if (!locatorsConfigValue.contains(thisLocator)) {
-            locatorsConfigValue = locatorsConfigValue + ',' + thisLocator;
-            setLocatorsProp = true;
-          }
-        } else {
-          locatorsConfigValue = thisLocator;
-          setLocatorsProp = true;
-        }
-        if (setLocatorsProp) {
-          Properties updateEnv = new Properties();
-          updateEnv.setProperty(LOCATORS, locatorsConfigValue);
-          distributionConfig.setApiProps(updateEnv);
-          String locatorsPropertyName = GEMFIRE_PREFIX + LOCATORS;
-          if (System.getProperty(locatorsPropertyName) != null) {
-            System.setProperty(locatorsPropertyName, locatorsConfigValue);
-          }
-        }
-        // No longer default mcast-port to zero.
+    HostAddress thisLocator = getHostAddress(hostAddress);
+    if (peerLocator) {
+      String existingLocators = distributionConfig.getLocators();
+      String newLocators = addLocatorIfMissing(existingLocators, thisLocator, getPort());
+      if (!newLocators.equals(existingLocators)) {
+        setLocatorProperties(newLocators);
       }
+    }
 
-      Properties distributedSystemProperties = new Properties();
-      // LogWriterAppender is now shared via that class
-      // using a DistributionConfig earlier in this method
-      distributedSystemProperties.put(DistributionConfig.DS_CONFIG_NAME, distributionConfig);
-
-      logger.info("Starting distributed system");
-
-      internalDistributedSystem =
-          InternalDistributedSystem
-              .connectInternal(distributedSystemProperties, null,
-                  new InternalDistributedSystemMetricsService.Builder(),
-                  membershipLocator);
-
-      if (peerLocator) {
-        // We've created a peer location message handler - it needs to be connected to
-        // the membership service in order to get membership view notifications
-        membershipLocator
-            .setMembership(internalDistributedSystem.getDM()
-                .getDistribution().getMembership());
-      }
+    Properties distributedSystemProperties = new Properties();
+    // LogWriterAppender is now shared via that class
+    // using a DistributionConfig earlier in this method
+    distributedSystemProperties.put(DistributionConfig.DS_CONFIG_NAME, distributionConfig);
+
+    logger.info("Starting distributed system");
+
+    internalDistributedSystem =
+        InternalDistributedSystem
+            .connectInternal(distributedSystemProperties, null,
+                new InternalDistributedSystemMetricsService.Builder(),
+                membershipLocator);
+
+    if (peerLocator) {
+      // We've created a peer location message handler - it needs to be connected to
+      // the membership service in order to get membership view notifications
+      membershipLocator
+          .setMembership(internalDistributedSystem.getDM()
+              .getDistribution().getMembership());
+    }
+
+    internalDistributedSystem.addDisconnectListener(sys -> stop(false, false, false));
+
+    startCache(internalDistributedSystem);
 
-      internalDistributedSystem.addDisconnectListener(sys -> stop(false, false, false));
+    logger.info("Locator started on {}[{}]", thisLocator, getPort());
 
-      startCache(internalDistributedSystem);
+  }
+
+  HostAddress getHostAddress(HostAddress hostAddress) throws UnknownHostException {
+    HostAddress thisLocator;
+    if (hostAddress != null && !StringUtils.isEmpty(hostAddress.getHostName())) {
+      thisLocator = hostAddress;
+    } else {
+      thisLocator = new HostAddress(LocalHostUtil.getLocalHost());
+    }

Review Comment:
   I believe the reason for that is this list is for peers, not for clients. The original code doesn't consider the `hostname-for-cleints`, so I am leaving it out here as well. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org