You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/12/12 08:07:04 UTC
[GitHub] [geode] mivanac opened a new pull request #5843: Feature/geode 8768
mivanac opened a new pull request #5843:
URL: https://github.com/apache/geode/pull/5843
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [*] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [*] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [*] Is your initial contribution a single, squashed commit?
- [*] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #5843:
URL: https://github.com/apache/geode/pull/5843#discussion_r568179833
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
##########
@@ -317,6 +363,43 @@ public boolean equals(Object other) {
return false;
final DistributionLocatorId that = (DistributionLocatorId) other;
+ if (this.membername != null && that.membername != null) {
+ if (this.membername.equals(that.membername))
+ return true;
+
+ return false;
+ }
+
+ if (!StringUtils.equals(this.hostnameForClients, that.hostnameForClients))
+ return false;
+ if (this.host != that.host && !(this.host != null && this.host.equals(that.host)))
Review comment:
I think it should be "||", not "&&".
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5843:
URL: https://github.com/apache/geode/pull/5843#discussion_r564854356
##########
File path: geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/misc/WanLocatorDiscoveryDUnitTest.java
##########
@@ -0,0 +1,132 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
+ * agreements. See the NOTICE file distributed with this work for additional information regarding
+ * copyright ownership. The ASF licenses this file to You under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance with the License. You may obtain a
+ * copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License
+ * is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express
+ * or implied. See the License for the specific language governing permissions and limitations under
+ * the License.
+ */
+package org.apache.geode.internal.cache.wan.misc;
+
+import static org.apache.geode.distributed.ConfigurationProperties.DISTRIBUTED_SYSTEM_ID;
+import static org.apache.geode.distributed.ConfigurationProperties.MCAST_PORT;
+import static org.apache.geode.distributed.ConfigurationProperties.REMOTE_LOCATORS;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import java.io.IOException;
+import java.util.Map;
+import java.util.Properties;
+import java.util.Set;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.client.internal.locator.wan.LocatorMembershipListener;
+import org.apache.geode.internal.AvailablePortHelper;
+import org.apache.geode.internal.admin.remote.DistributionLocatorId;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.IgnoredException;
+import org.apache.geode.test.dunit.rules.ClusterStartupRule;
+import org.apache.geode.test.dunit.rules.MemberVM;
+
+public class WanLocatorDiscoveryDUnitTest {
+
+ private static MemberVM locator_ln1;
+ private static MemberVM locator_ln2;
+
+ private static MemberVM locator_ny1;
+ private static MemberVM locator_ny2;
+ private int[] ports;
+
+ @Rule
+ public ClusterStartupRule cluster = new ClusterStartupRule();
+
+ @Before
+ public void setupCluster() throws Exception {
+ IgnoredException.addIgnoredException("Connection reset");
+ IgnoredException.addIgnoredException("Broken pipe");
+ IgnoredException.addIgnoredException("Connection refused");
+ IgnoredException.addIgnoredException("could not get remote locator information");
+ IgnoredException.addIgnoredException("Unexpected IOException");
+ }
+
+ private void setupWanSites() throws IOException {
+ ports = AvailablePortHelper.getRandomAvailableTCPPorts(5);
+ int site1Port =
+ setupWanSite1();
+ setupWanSite2(site1Port);
+ }
+
+ private int setupWanSite1() throws IOException {
+ Properties locator_ln_props = new Properties();
+ // create a cluster
+ locator_ln_props.setProperty(MCAST_PORT, "0");
Review comment:
You don't need all these when you use ClusterStartupRule. The rule sets those properties for you already so that you can avoid all these boiler code.
The rule also allocates the available ports for you so that you don't need to pre-allocate them.
use something like:
locator = cluster.startLocatorVM(index, l->l.withPropety(....).withConnectionToLocator(port0, port1, port2));
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #5843:
URL: https://github.com/apache/geode/pull/5843#discussion_r568174860
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
##########
@@ -104,6 +122,26 @@ public DistributionLocatorId(InetAddress host, int port, String bindAddress, SSL
* two.
*/
public DistributionLocatorId(String marshalled) {
+ this(marshalled, null);
+ }
+
+ /**
+ * Constructs a DistributionLocatorId with a String of the form: hostname[port] or
+ * hostname:bindaddress[port] or hostname@bindaddress[port]
+ * and membername
+ * <p>
+ * The :bindaddress portion is optional. hostname[port] is the more common form.
+ * <p>
+ * Example: merry.gemstone.com[7056]<br>
+ * Example w/ bind address: merry.gemstone.com:81.240.0.1[7056], or
+ * merry.gemstone.com@fdf0:76cf:a0ed:9449::16[7056]
Review comment:
is this "::" a typo here?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #5843:
URL: https://github.com/apache/geode/pull/5843#discussion_r568175831
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
##########
@@ -104,6 +122,26 @@ public DistributionLocatorId(InetAddress host, int port, String bindAddress, SSL
* two.
*/
public DistributionLocatorId(String marshalled) {
+ this(marshalled, null);
+ }
+
+ /**
+ * Constructs a DistributionLocatorId with a String of the form: hostname[port] or
+ * hostname:bindaddress[port] or hostname@bindaddress[port]
+ * and membername
Review comment:
it's better to put an example of membername here.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] jinmeiliao commented on a change in pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
jinmeiliao commented on a change in pull request #5843:
URL: https://github.com/apache/geode/pull/5843#discussion_r565462202
##########
File path: geode-wan/src/distributedTest/java/org/apache/geode/internal/cache/wan/misc/WanLocatorDiscoveryDUnitTest.java
##########
@@ -65,25 +65,29 @@ private void setupWanSites() throws IOException {
}
private int setupWanSite1() throws IOException {
- Properties locator_ln_props = new Properties();
- // create a cluster
- locator_ln_props.setProperty(MCAST_PORT, "0");
- locator_ln_props.setProperty(DISTRIBUTED_SYSTEM_ID, "1");
-
- locator_ln1 = cluster.startLocatorVM(0, ports[0], locator_ln_props);
- locator_ln2 = cluster.startLocatorVM(1, ports[1], locator_ln_props, locator_ln1.getPort());
- return locator_ln1.getPort();
+ locator_ln1 = cluster.startLocatorVM(0, ports[0], VersionManager.CURRENT_VERSION,
+ i -> i.withProperty(MCAST_PORT, "0").withProperty(DISTRIBUTED_SYSTEM_ID, "1"));
Review comment:
you can get rid of `withProperty(MCAST_PORT, "0")`, it's already set by the rule
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-775426324
On looking at the changes further, I think that DistributionLocatorId.equals() should treat a membername holding an empty string as being the same as a null. There is code in WanDiscovery, at least, that gets the membername from a MemberIdentifierImpl, and that class will return an empty string if the member name is null.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #5843:
URL: https://github.com/apache/geode/pull/5843#discussion_r568179833
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
##########
@@ -317,6 +363,43 @@ public boolean equals(Object other) {
return false;
final DistributionLocatorId that = (DistributionLocatorId) other;
+ if (this.membername != null && that.membername != null) {
+ if (this.membername.equals(that.membername))
+ return true;
+
+ return false;
+ }
+
+ if (!StringUtils.equals(this.hostnameForClients, that.hostnameForClients))
+ return false;
+ if (this.host != that.host && !(this.host != null && this.host.equals(that.host)))
Review comment:
Should it be:
if (this.host != null && !this.host.equals(that.host))
?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] pivotal-amurmann commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
pivotal-amurmann commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-767030985
Adding folks who according to CODEOWNERS should review this.
@jinmeiliao @jdeppe-pivotal @kirklund
@boglesby @gesterzhou
@bschuchardt @Bill
Wow, lots of owners for one PR!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] pivotal-amurmann commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
pivotal-amurmann commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-767030985
Adding folks who according to CODEOWNERS should review this.
@jinmeiliao @jdeppe-pivotal @kirklund
@boglesby @gesterzhou
@bschuchardt @Bill
Wow, lots of owners for one PR!
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-775737592
If this is creating problems in your tests, I will need to modify logic.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-753862169
This PR is ready to be reviewed.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #5843:
URL: https://github.com/apache/geode/pull/5843#discussion_r568259267
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
##########
@@ -317,6 +363,43 @@ public boolean equals(Object other) {
return false;
final DistributionLocatorId that = (DistributionLocatorId) other;
+ if (this.membername != null && that.membername != null) {
+ if (this.membername.equals(that.membername))
+ return true;
+
+ return false;
+ }
+
+ if (!StringUtils.equals(this.hostnameForClients, that.hostnameForClients))
+ return false;
+ if (this.host != that.host && !(this.host != null && this.host.equals(that.host)))
Review comment:
Bruce suggested to use:
if (!Objects.equals(this.host, that.host) {
return false;
}
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-775433042
Thanks for info. I will add changes.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] ladyVader commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
ladyVader commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-775518895
It seems that DistributionLocatorId.equals() can return true if one of the two compared objects doesn't have the memberName and the other does ... but DistributionLocatorId.hashCode() will be different ... if we invoke contains() on a Set of DistributionLocatorIds ... contains() will return false in that case.
I also found the logging a bit misleading once toString() didn't include the memberName.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] gesterzhou commented on a change in pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
gesterzhou commented on a change in pull request #5843:
URL: https://github.com/apache/geode/pull/5843#discussion_r568179833
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
##########
@@ -317,6 +363,43 @@ public boolean equals(Object other) {
return false;
final DistributionLocatorId that = (DistributionLocatorId) other;
+ if (this.membername != null && that.membername != null) {
+ if (this.membername.equals(that.membername))
+ return true;
+
+ return false;
+ }
+
+ if (!StringUtils.equals(this.hostnameForClients, that.hostnameForClients))
+ return false;
+ if (this.host != that.host && !(this.host != null && this.host.equals(that.host)))
Review comment:
Should it be:
if (this.host != null && !this.host.equals(that.host))
?
##########
File path: geode-core/src/main/java/org/apache/geode/internal/admin/remote/DistributionLocatorId.java
##########
@@ -317,6 +363,43 @@ public boolean equals(Object other) {
return false;
final DistributionLocatorId that = (DistributionLocatorId) other;
+ if (this.membername != null && that.membername != null) {
+ if (this.membername.equals(that.membername))
+ return true;
+
+ return false;
+ }
+
+ if (!StringUtils.equals(this.hostnameForClients, that.hostnameForClients))
+ return false;
+ if (this.host != that.host && !(this.host != null && this.host.equals(that.host)))
Review comment:
Bruce suggested to use:
if (!Objects.equals(this.host, that.host) {
return false;
}
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] kohlmu-pivotal commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
kohlmu-pivotal commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-753690592
@mivanac Is this a "work in progress" PR? If so, please change its status to "Draft".
Otherwise, @nabarunnag @agingade would is be possible to review these changes?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] bschuchardt commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-776071182
@mivanac I think the best thing to do at this point is revert the merge and write some tests to fix the problems with
1. membername being an empty string. In this case all other fields are ignored.
2. hashcode returning a value that places IDs having/not-having membernames in different hashmap buckets
I noticed #1 when reviewing code when people were talking about #2. I wrote a test for #1. A test for #2 is harder to write.
DistributionLocatorIdJUnitTest.java
```java
@Test
public void testEqualsWithMemberId() throws UnknownHostException {
InetAddress address = InetAddress.getLocalHost();
DistributionLocatorId dLI1 = new DistributionLocatorId(address, 40404, "127.0.0.1", null);
DistributionLocatorId dLI2 = dLI1;
@SuppressWarnings("RedundantStringConstructorCall")
DistributionLocatorId dLI3 =
new DistributionLocatorId(address, 40404, new String("127.0.0.1"), null);
@SuppressWarnings("RedundantStringConstructorCall")
DistributionLocatorId dLI4 = new DistributionLocatorId(InetAddress.getByName("localhost"),
40405, new String("127.0.0.1"), null);
dLI1.setMembername("");
dLI3.setMembername("");
dLI4.setMembername("");
assertTrue(dLI1.equals(dLI2));
assertTrue(dLI1.equals(dLI3));
assertFalse(dLI1.equals(dLI4));
}
```
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] mivanac merged pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
mivanac merged pull request #5843:
URL: https://github.com/apache/geode/pull/5843
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [geode] mivanac commented on pull request #5843: Feature/geode 8768
Posted by GitBox <gi...@apache.org>.
mivanac commented on pull request #5843:
URL: https://github.com/apache/geode/pull/5843#issuecomment-775719885
Equals() and hashCode(), are modified to support cases, when sites with old implementation are discovered with new locators. Also this is the reason why membername is not included in toString().
----------------------------------------------------------------
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:
users@infra.apache.org