bschuchardt commented on a change in pull request #6013:
URL: https://github.com/apache/geode/pull/6013#discussion_r574091367
##########
File path:
geode-wan/src/main/java/org/apache/geode/cache/client/internal/locator/wan/LocatorHelper.java
##########
@@ -46,13 +46,42 @@ public static boolean addLocator(int distributedSystemId,
DistributionLocatorId
Set<DistributionLocatorId> existingValue =
allLocatorsInfo.putIfAbsent(distributedSystemId, locatorsSet);
if (existingValue != null) {
- if (!existingValue.contains(locator)) {
+ if (locator.getMemberName() != null) {
+ DistributionLocatorId tempLocator = null;
Review comment:
This could use a comment about why the code is looking for an old ID
with the same member name and removing it. A better variable name than
tempLocator would help, too.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
##########
@@ -305,7 +354,11 @@ public String toString() {
* Indicates whether some other object is "equal to" this one.
*
* @param other the reference object with which to compare.
- * @return true if this object is the same as the obj argument; false
otherwise.
+ *
+ * If member name is defined in both objects, and both objects have
same member name,
Review comment:
this javadoc change no longer applies since the equals() method no
longer checks member names
##########
File path:
geode-wan/src/main/java/org/apache/geode/cache/client/internal/locator/wan/LocatorHelper.java
##########
@@ -46,13 +46,42 @@ public static boolean addLocator(int distributedSystemId,
DistributionLocatorId
Set<DistributionLocatorId> existingValue =
allLocatorsInfo.putIfAbsent(distributedSystemId, locatorsSet);
if (existingValue != null) {
- if (!existingValue.contains(locator)) {
+ if (locator.getMemberName() != null) {
+ DistributionLocatorId tempLocator = null;
+ for (DistributionLocatorId locElement : existingValue) {
Review comment:
This code is pretty much duplicated below. It might read better if you
extracted it to a method like getLocatorWithSameMemberName(locatorId,
locatorIdCollection)
##########
File path:
geode-wan/src/main/java/org/apache/geode/cache/client/internal/locator/wan/WanLocatorDiscovererImpl.java
##########
@@ -65,11 +66,19 @@ private void exchangeLocalLocators(int port,
DistributionConfigImpl config,
LocatorMembershipListener locatorListener, final String
hostnameForClients) {
String localLocator = config.getStartLocator();
DistributionLocatorId locatorId = null;
+
+ InternalDistributedSystem system =
InternalDistributedSystem.getConnectedInstance();
Review comment:
same comment - we want to stop using this static method to find an
InternalDistributedSystem, and you have a DistributionConfig parameter that
provides the memberName that you need.
##########
File path:
geode-wan/src/main/java/org/apache/geode/cache/client/internal/locator/wan/LocatorMembershipListenerImpl.java
##########
@@ -108,11 +109,18 @@ public void locatorJoined(final int distributedSystemId,
final DistributionLocat
final DistributionLocatorId sourceLocator) {
// DistributionLocatorId for local locator.
DistributionLocatorId localLocatorId;
+
+ InternalDistributedSystem system =
InternalDistributedSystem.getConnectedInstance();
Review comment:
we're trying to get rid of the use of static methods like this. The
DistributionConfig argument passed to this method should have the memberName in
it. You should use that instead.
##########
File path:
geode-wan/src/main/java/org/apache/geode/cache/client/internal/locator/wan/LocatorHelper.java
##########
@@ -46,13 +46,42 @@ public static boolean addLocator(int distributedSystemId,
DistributionLocatorId
Set<DistributionLocatorId> existingValue =
allLocatorsInfo.putIfAbsent(distributedSystemId, locatorsSet);
if (existingValue != null) {
- if (!existingValue.contains(locator)) {
+ if (locator.getMemberName() != null) {
Review comment:
Mario, I think you still need need to check for an empty string and
treat that the same as a null. Maybe it would be easier to have the default
value be an empty string in DistributionLocatorId. Then you could check for an
empty string and not need a null check.
##########
File path:
geode-wan/src/main/java/org/apache/geode/cache/client/internal/locator/wan/LocatorHelper.java
##########
@@ -100,6 +129,17 @@ public static boolean addExchangedLocators(Map<Integer,
Set<DistributionLocatorI
if (!localLocators.equals(entry.getValue())) {
entry.getValue().removeAll(localLocators);
for (DistributionLocatorId locator : entry.getValue()) {
+ if (locator.getMemberName() != null && !localLocators.isEmpty())
{
+ boolean locatorExist = false;
+ for (DistributionLocatorId locId : localLocators) {
Review comment:
duplicate code - also this could use stream()->findFirst() instead of
iterating
##########
File path:
geode-wan/src/main/java/org/apache/geode/cache/client/internal/locator/wan/WanLocatorDiscovererImpl.java
##########
@@ -113,10 +122,17 @@ private RemoteLocatorJoinRequest
buildRemoteDSJoinRequest(int port, Distribution
final String hostnameForClients) {
String localLocator = config.getStartLocator();
DistributionLocatorId locatorId = null;
+
+ InternalDistributedSystem system =
InternalDistributedSystem.getConnectedInstance();
Review comment:
same comment about this method and the config parameter
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
