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

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

jinmeiliao commented on PR #7703:
URL: https://github.com/apache/geode/pull/7703#issuecomment-1131987304

   > The scope of this problem is in question. At the highest level it's not clear why (before this PR) it is necessary for an `InternalLocator` to add itself to the effective set of locators. The product has dynamic discovery features for locators across WAN, and I believe within the cluster too (TBD on that second point).
   > 
   > If we decide it's really necessary to add the current locator to that set, the next question is: why are we doing it by modifying the `DistributionConfig.locators` and Java system property `gemfire.locators`. Those should be _immutable_. If we need to have a set of all known locators, it seems they should be managed in collection separate from the immutable configuration settings.
   > 
   > If we assume that we do need to add the current locator to the set of known locators (as is done in this PR) there are some low-level coding considerations:
   > 
   > 1. The PR uses string comparison to match locators. Rather than comparing strings, I believe we need to be comparing the entries in terms of their semantic components (host/address and port). We have numerous parsers for this locators format and I believe some allow whitespace around e.g. `[` and `]`. A naïve string comparison will be inaccurate. It appears that you started to do things this way (based on your refactoring of the parser in `GMSUtil` and then abandoned that change. I think that refactoring is valuable since it lets you avoid some unneeded functionality in `GMSUtil`.
   > 2. If we need side-effects, e.g. modifying the `DistributionConfig` and setting a Java system property, rather than burying it in a call chain: `getLocatorString` -> `configurePeerLocators` -> `setLocatorProperties`, it would be more understandable, and more testable to split the job into methods that compute new values without any side-effects, and methods that have side-effects. The latter should be named so it is clear that they have side-effects. Maybe we could have something like: `String addLocatorIfMissing(final String locators, final HostAndPort theLocator)` (a side-effect-free method) and the caller could compare the result to current value and if it differs it could call `void setLocatorProperties(String locatorsConfigValue)` (clearly a method with side-effects).
   
   I abandoned using the `GMSUtils` because it's an internal api in Membership. If I use that in the core module, it will break the api check. Do you want me to make GMSUtils to be public?
   
   Regarding the scope of the PR, I want to keep it to a minimal at this stage. We need to fix a specific problem introduced by 1.12 (the duplicate entry in the locators list). As far as whether we need to mutate the configuration or not, I believe that's another discussion.


-- 
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