You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by GitBox <gi...@apache.org> on 2020/04/21 12:03:35 UTC
[GitHub] [geode] alb3rtobr opened a new pull request #4978: Fix for regression introduced by GEODE-7565
alb3rtobr opened a new pull request #4978:
URL: https://github.com/apache/geode/pull/4978
This PR reverts the revert of GEODE-7565 and introduces a fix for the problem identified
----------------------------------------------------------------
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] jujoramos commented on issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-617814149
One additional comment regarding the following waning message:
```
[warn 2020/04/18 23:44:22.757 PDT <ServerConnection on port 29019 Thread 1> tid=0x298] Unable to ping non-member rs-FullRegression19040559a2i32xlarge-hydra-client-63(bridgegemfire1_host1_4749:4749)<ec><v39>:41003 for client identity(rs-FullRegression19040559a2i32xlarge-hydra-client-63(edgegemfire3_host1_1071:1071:loner):50046:5a182991:edgegemfire3_host1_1071,connection=2
```
The above is logged by member `bridgegemfire1_host1_4749` in the tests, exactly the member to which the `ping` command was sent... this warning is logged within the `pingCorrectServer` method which, unless I'm missing something, shouldn't be invoked at all if this member is the actual recipient for the `ping` message. Maybe `!myID.equals(targetServer)` is not working as expected?.
----------------------------------------------------------------
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] jujoramos commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-622309762
Great, I'll test the changes and review the PR afterwards.
Cheers.
----------------------------------------------------------------
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] jujoramos commented on issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-618315601
@alb3rtobr @bschuchardt : added some more analysis and comments regarding one of the failures [here](https://issues.apache.org/jira/browse/GEODE-8004?focusedCommentId=17090468&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17090468).
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418631463
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/PingTest.java
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.tier.sockets.command;
+
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.tier.sockets.Message;
+import org.apache.geode.internal.cache.tier.sockets.Part;
+import org.apache.geode.internal.cache.tier.sockets.ServerConnection;
+
+public class PingTest {
Review comment:
done!
----------------------------------------------------------------
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] jujoramos commented on issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-618449100
@alb3rtobr
I've been discussing the issue with @bschuchardt and we think you can use `MemberIdentifier.compareTo(other, compareMemberData, compareViewIDs)` to execute the member comparison instead of `equals`, that would guarantee that the `ping` is handled by the current member when the only difference between `myID` and `targetServer` is the `viewId`.
Sounds feasible?.
----------------------------------------------------------------
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] alb3rtobr commented on pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-619030061
> @alb3rtobr: I've executed the tests again and still see some failures, see my inline comments within the `Ping.java` class.
> As a side note, I still don't see tests added to this pull request (only one new, which is really scarce considering the amount of changes introduced through `GEODE-7565`), please add multiple tests to verify all the code changes and additions.
I hope last two commits solve the failure I introduced, sorry for that.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r412828019
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -43,12 +45,16 @@ private PingOp() {
static class PingOpImpl extends AbstractOp {
private long startTime;
+ private ServerLocation location;
Review comment:
The above fields are not used anywhere, can we just remove them?, or am I missing something?.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -65,8 +71,9 @@ protected boolean needsUserId() {
@Override
protected void sendMessage(Connection cnx) throws Exception {
getMessage().clearMessageHasSecurePartFlag();
- this.startTime = System.currentTimeMillis();
- getMessage().send(false);
+ getMessage().setNumberOfParts(1);
+ getMessage().addObjPart(serverID);
+ getMessage().send(true);
Review comment:
These operations can be directly executed within the `PingOpImpl` constructor, as we do with the rest of the messages. I've also noted that you changed the `clearMessage` flag from `false` to `true` as well, any reasons behind that?.
----------------------------------------------------------------
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] jujoramos edited a comment on issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos edited a comment on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-618315601
@alb3rtobr @bschuchardt : added some more analysis and comments regarding one of the failures [here](https://issues.apache.org/jira/browse/GEODE-8004?focusedCommentId=17090468&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17090468).
I will keep analysing the other issue (harder to reproduce) and update the ticket as soon as I have something to share.
----------------------------------------------------------------
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] alb3rtobr commented on pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-620729123
> I've added a small `DistributedTest` to my [branch](https://github.com/apache/geode/pull/4990/files#diff-b478e9ee8bbd7e0de69c1bac78ee0db2), you can use it as the starting point to test the different scenarios of the `Ping` execution. The method [`memberShouldNotRedirectPingMessageWhenClientCachedViewIdIsWrong`](https://github.com/apache/geode/pull/4990/files#diff-b478e9ee8bbd7e0de69c1bac78ee0db2R188) reproduces exactly the `ClassCastException` I'm seeing during our internal testing.
Thanks @jujoramos , I have used your code to create a test and identify the problem. I will add more tests as requested.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r420103151
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -497,13 +558,21 @@ private void updateMap(Map map, ServerLocation location, float load, float loadP
* If it is most loaded then return its LoadHolder; otherwise return null;
*/
private LoadHolder isCurrentServerMostLoaded(ServerLocation currentServer,
- Map<ServerLocation, LoadHolder> groupServers) {
- final LoadHolder currentLH = groupServers.get(currentServer);
+ Map<ServerLocationAndMemberId, LoadHolder> groupServers) {
Review comment:
done!
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418041985
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedPingMessage.java
##########
@@ -0,0 +1,79 @@
+/*
+ * 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;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.internal.ClusterDistributionManager;
+import org.apache.geode.distributed.internal.HighPriorityDistributionMessage;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.tier.sockets.ClientHealthMonitor;
+import org.apache.geode.internal.cache.tier.sockets.ClientProxyMembershipID;
+import org.apache.geode.internal.serialization.DeserializationContext;
+import org.apache.geode.internal.serialization.SerializationContext;
+import org.apache.geode.internal.serialization.Version;
+
+public class DistributedPingMessage extends HighPriorityDistributionMessage {
Review comment:
Do you think its enough with the distributed tests added? I think the class so simple that there is nothing I could test with unit test that is not already tested by the distributed tests.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r414405668
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -65,8 +71,9 @@ protected boolean needsUserId() {
@Override
protected void sendMessage(Connection cnx) throws Exception {
getMessage().clearMessageHasSecurePartFlag();
- this.startTime = System.currentTimeMillis();
- getMessage().send(false);
+ getMessage().setNumberOfParts(1);
+ getMessage().addObjPart(serverID);
+ getMessage().send(true);
Review comment:
thanks @bschuchardt
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418051895
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedPingMessage.java
##########
@@ -0,0 +1,79 @@
+/*
+ * 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;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.internal.ClusterDistributionManager;
+import org.apache.geode.distributed.internal.HighPriorityDistributionMessage;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.tier.sockets.ClientHealthMonitor;
+import org.apache.geode.internal.cache.tier.sockets.ClientProxyMembershipID;
+import org.apache.geode.internal.serialization.DeserializationContext;
+import org.apache.geode.internal.serialization.SerializationContext;
+import org.apache.geode.internal.serialization.Version;
+
+public class DistributedPingMessage extends HighPriorityDistributionMessage {
Review comment:
Hey @alb3rtobr
Agreed, the class itself is pretty simple... Having extra unit tests is harmless (and beneficial), however, so I'd say go ahead and add them, it shouldn't take long :-).
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418578669
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/GridAdvisor.java
##########
@@ -418,18 +418,24 @@ public String toString() {
public int hashCode() {
final String thisHost = this.gp.getHost();
final int thisPort = this.gp.getPort();
- return thisHost != null ? (thisHost.hashCode() ^ thisPort) : thisPort;
+ final String thisMemberId = this.getMemberId().getUniqueId();
+ final int thisMemberIdHashCode = (thisMemberId != null) ? thisMemberId.hashCode() : 0;
+ return thisHost != null ? (thisHost.hashCode() ^ thisPort) + thisMemberIdHashCode
+ : thisPort + thisMemberIdHashCode;
}
@Override
public boolean equals(Object obj) {
if (obj instanceof GridProfileId) {
final GridProfileId other = (GridProfileId) obj;
+
if (this.gp.getPort() == other.gp.getPort()) {
final String thisHost = this.gp.getHost();
final String otherHost = other.gp.getHost();
if (thisHost != null) {
- return thisHost.equals(otherHost);
+ if (thisHost.equals(otherHost)) {
+ return this.getMemberId().equals(other.getMemberId());
Review comment:
Good catch!
----------------------------------------------------------------
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] jujoramos commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-622298896
Hello @alb3rtobr
Thanks so much for all your hard work on this feature!. Please let me know once you're done so I can run the original failing test and re-review the PR.
Cheers.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r419999012
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ServerLocationAndMemberId.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.distributed.internal;
+
+public class ServerLocationAndMemberId {
+
+ private final ServerLocation serverLocation;
+ private final String memberId;
+
+ public ServerLocationAndMemberId() {
Review comment:
This constructor doesn't seem to be used anywhere, so we could just delete it.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -425,6 +429,22 @@ private void addGroups(Map<String, Map<ServerLocation, LoadHolder>> map, String[
}
}
+ private void addGroups(Map<String, Map<ServerLocationAndMemberId, LoadHolder>> map,
Review comment:
This is a new method and should be, at leat, unit tested.
You can make it package private, annotate it with `@VisibleForTesting` and access it directly from `LocatorLoadSnapshotJUnitTest` and/or `LocatorLoadSnapshotIntegrationTest`.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -440,6 +460,24 @@ private void removeFromMap(Map<String, Map<ServerLocation, LoadHolder>> map, Str
groupMap.remove(location);
}
+ private void removeFromMap(Map<String, Map<ServerLocationAndMemberId, LoadHolder>> map,
Review comment:
This is a new method and should be, at leat, unit tested.
You can make it package private, annotate it with `@VisibleForTesting` and access it directly from `LocatorLoadSnapshotJUnitTest` and/or `LocatorLoadSnapshotIntegrationTest`.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -448,14 +486,28 @@ private void updateMap(Map map, ServerLocation location, float load, float loadP
}
}
+ private void updateMap(Map map, ServerLocation location, String memberId, float load,
+ float loadPerConnection) {
+ Map groupMap = (Map) map.get(null);
+ ServerLocationAndMemberId locationAndMemberId =
+ new ServerLocationAndMemberId(location, memberId);
+ LoadHolder holder =
+ (LoadHolder) groupMap.get(locationAndMemberId);
+
+ if (holder != null) {
+ holder.setLoad(load, loadPerConnection);
+ }
+ }
+
/**
*
* @param groupServers the servers to consider
* @param excludedServers servers to exclude
* @param count how many you want. a negative number means all of them in order of best to worst
* @return a list of best...worst server LoadHolders
*/
- private List<LoadHolder> findBestServers(Map<ServerLocation, LoadHolder> groupServers,
+ private List<LoadHolder> findBestServers(
Review comment:
Not a new method but significantly changed, it should be, at leat, unit tested.
You can make it package private, annotate it with `@VisibleForTesting` and access it directly from `LocatorLoadSnapshotJUnitTest` and/or `LocatorLoadSnapshotIntegrationTest`.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -497,13 +558,21 @@ private void updateMap(Map map, ServerLocation location, float load, float loadP
* If it is most loaded then return its LoadHolder; otherwise return null;
*/
private LoadHolder isCurrentServerMostLoaded(ServerLocation currentServer,
- Map<ServerLocation, LoadHolder> groupServers) {
- final LoadHolder currentLH = groupServers.get(currentServer);
+ Map<ServerLocationAndMemberId, LoadHolder> groupServers) {
Review comment:
Not a new method but significantly changed, it should be, at leat, unit tested.
You can make it package private, annotate it with `@VisibleForTesting` and access it directly from `LocatorLoadSnapshotJUnitTest` and/or `LocatorLoadSnapshotIntegrationTest`.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -448,14 +486,28 @@ private void updateMap(Map map, ServerLocation location, float load, float loadP
}
}
+ private void updateMap(Map map, ServerLocation location, String memberId, float load,
Review comment:
This is a new method and should be, at leat, unit tested.
You can make it package private, annotate it with `@VisibleForTesting` and access it directly from `LocatorLoadSnapshotJUnitTest` and/or `LocatorLoadSnapshotIntegrationTest`.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r414140568
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -65,8 +71,9 @@ protected boolean needsUserId() {
@Override
protected void sendMessage(Connection cnx) throws Exception {
getMessage().clearMessageHasSecurePartFlag();
- this.startTime = System.currentTimeMillis();
- getMessage().send(false);
+ getMessage().setNumberOfParts(1);
+ getMessage().addObjPart(serverID);
+ getMessage().send(true);
Review comment:
Im afraid I dont have a reason to explain that change. I used a draft from @bschuchardt when implementing this, and I didnt realize about it. Bruce, could you help with this please?
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r414567295
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Ping.java
##########
@@ -55,12 +55,11 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
(InternalDistributedMember) clientMessage.getPart(0).getObject();
InternalDistributedMember myID = serverConnection.getCache().getMyId();
if (!myID.equals(targetServer)) {
- if (myID.compareTo(targetServer.getMemberIdentifier(), true, false) == 0) {
+ if (myID.compareTo(targetServer, true, false) == 0) {
logger.warn("Target server {} has different viewId {}", targetServer, myID);
writeErrorResponse(clientMessage, MessageType.EXCEPTION, serverConnection);
} else {
pingCorrectServer(clientMessage, targetServer, serverConnection);
- writeReply(clientMessage, serverConnection);
Review comment:
@jujoramos Implementing tests I realized this writeReply here is duplicated, `pingCorrectServer` is already calling it when the ping is forwarded. So this should be the cause for the error you saw about an unexpected REPLY message.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r415863628
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -50,7 +50,8 @@
private final Map<ServerLocation, String[]> serverGroupMap = new HashMap<>();
- private final Map<String, Map<ServerLocation, LoadHolder>> connectionLoadMap = new HashMap<>();
+ private final Map<String, Map<ServerLocationAndMemberId, LoadHolder>> connectionLoadMap =
Review comment:
This entire class has several modifications and new non-trivial methods, we should add several tests to verify the correct behaviour.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedPingMessage.java
##########
@@ -0,0 +1,79 @@
+/*
+ * 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;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.internal.ClusterDistributionManager;
+import org.apache.geode.distributed.internal.HighPriorityDistributionMessage;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.tier.sockets.ClientHealthMonitor;
+import org.apache.geode.internal.cache.tier.sockets.ClientProxyMembershipID;
+import org.apache.geode.internal.serialization.DeserializationContext;
+import org.apache.geode.internal.serialization.SerializationContext;
+import org.apache.geode.internal.serialization.Version;
+
+public class DistributedPingMessage extends HighPriorityDistributionMessage {
Review comment:
This is a new class, unit and distribution tests are required.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
##########
@@ -641,4 +641,5 @@ public UUID getUUID() {
public interface HostnameResolver {
InetAddress getInetAddress(ServerLocation location) throws UnknownHostException;
}
+
Review comment:
Unnecessary empty line 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] jujoramos commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-625809319
@alb3rtobr
Didn't notice that, sorry, the `PR` is now merged and the ticket closed, thanks a lot for your work!.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r420103367
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -448,14 +486,28 @@ private void updateMap(Map map, ServerLocation location, float load, float loadP
}
}
+ private void updateMap(Map map, ServerLocation location, String memberId, float load,
+ float loadPerConnection) {
+ Map groupMap = (Map) map.get(null);
+ ServerLocationAndMemberId locationAndMemberId =
+ new ServerLocationAndMemberId(location, memberId);
+ LoadHolder holder =
+ (LoadHolder) groupMap.get(locationAndMemberId);
+
+ if (holder != null) {
+ holder.setLoad(load, loadPerConnection);
+ }
+ }
+
/**
*
* @param groupServers the servers to consider
* @param excludedServers servers to exclude
* @param count how many you want. a negative number means all of them in order of best to worst
* @return a list of best...worst server LoadHolders
*/
- private List<LoadHolder> findBestServers(Map<ServerLocation, LoadHolder> groupServers,
+ private List<LoadHolder> findBestServers(
Review comment:
done
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -448,14 +486,28 @@ private void updateMap(Map map, ServerLocation location, float load, float loadP
}
}
+ private void updateMap(Map map, ServerLocation location, String memberId, float load,
Review comment:
done
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r415863628
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -50,7 +50,8 @@
private final Map<ServerLocation, String[]> serverGroupMap = new HashMap<>();
- private final Map<String, Map<ServerLocation, LoadHolder>> connectionLoadMap = new HashMap<>();
+ private final Map<String, Map<ServerLocationAndMemberId, LoadHolder>> connectionLoadMap =
Review comment:
This entire class has several modifications and new non-trivial methods, we should add tests to verify the correct behaviour.
----------------------------------------------------------------
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] alb3rtobr commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-622307657
> Hello @alb3rtobr
>
>
>
> Thanks so much for all your hard work on this feature!. Please let me know once you're done so I can run the original failing test and re-review the PR.
>
> Cheers.
I would say the PR is ready for testing, I think the error you reported is solved and covered by tests.
The only pending point are the tests for LocatorLoadSnapshot you requested. Although I have not added new tests, I had to adapt the existing ones because the new parts are used by them. Anyway yesterday I was trying to add tests for some new methods, but they use the LoadHolder class which is an inner class I cannot access in the tests (or I have not found the way yet).
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418579644
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/PingTest.java
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.tier.sockets.command;
+
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.tier.sockets.Message;
+import org.apache.geode.internal.cache.tier.sockets.Part;
+import org.apache.geode.internal.cache.tier.sockets.ServerConnection;
+
+public class PingTest {
Review comment:
Agreed, but you could easily move the whole logic about getting `ClientHealthMonitor` and updating the ping received to an extra method, which can be mocked and/or spied.
Another option would be to move the retrieval of the `ClientHealthMonitor` class to an extra method and mock that method to return your own spied `ClientHealthMonitor`, which can be checked afterwards.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r420281148
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ServerLocationAndMemberId.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.distributed.internal;
+
+public class ServerLocationAndMemberId {
+
+ private final ServerLocation serverLocation;
+ private final String memberId;
+
+ public ServerLocationAndMemberId(ServerLocation serverLocation, String memberId) {
+ this.serverLocation = serverLocation;
+ this.memberId = memberId;
+ }
+
+ public ServerLocation getServerLocation() {
+ return this.serverLocation;
+ }
+
+ public String getMemberId() {
+ return this.memberId;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj)
+ return true;
+ if (obj == null)
+ return false;
+ if (!(obj instanceof ServerLocationAndMemberId))
+ return false;
+ final ServerLocationAndMemberId other = (ServerLocationAndMemberId) obj;
+
+ if (!this.serverLocation.equals(other.getServerLocation())) {
+ return false;
+ }
+
+ return this.memberId.equals(other.getMemberId());
Review comment:
Fixed, thanks!
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/GridAdvisor.java
##########
@@ -418,18 +418,24 @@ public String toString() {
public int hashCode() {
final String thisHost = this.gp.getHost();
final int thisPort = this.gp.getPort();
- return thisHost != null ? (thisHost.hashCode() ^ thisPort) : thisPort;
+ final String thisMemberId = this.getMemberId().getUniqueId();
+ final int thisMemberIdHashCode = (thisMemberId != null) ? thisMemberId.hashCode() : 0;
+ return thisHost != null ? (thisHost.hashCode() ^ thisPort) + thisMemberIdHashCode
+ : thisPort + thisMemberIdHashCode;
}
@Override
public boolean equals(Object obj) {
if (obj instanceof GridProfileId) {
final GridProfileId other = (GridProfileId) obj;
+
if (this.gp.getPort() == other.gp.getPort()) {
final String thisHost = this.gp.getHost();
final String otherHost = other.gp.getHost();
if (thisHost != null) {
- return thisHost.equals(otherHost);
+ if (thisHost.equals(otherHost)) {
+ return this.getMemberId().getUniqueId().equals(other.getMemberId().getUniqueId());
Review comment:
Fixed, thanks!
----------------------------------------------------------------
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 issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-618488247
however, if that comparison returns true it should still result in an EXCEPTION being sent to the client so that the client knows that the server it intended to ping is no longer in the cluster. Then it can clean up its endpoint info for the old server.
----------------------------------------------------------------
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] jujoramos commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-625798962
@alb3rtobr
I think all's good from our side, you can go ahead, merge the `PR` and close the ticket. Ping me if you have any troubles in doing so.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r420103948
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ServerLocationAndMemberId.java
##########
@@ -0,0 +1,74 @@
+/*
+ * 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.distributed.internal;
+
+public class ServerLocationAndMemberId {
+
+ private final ServerLocation serverLocation;
+ private final String memberId;
+
+ public ServerLocationAndMemberId() {
Review comment:
done. After removing this constructor I was able to remove other one more.
----------------------------------------------------------------
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] alb3rtobr commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-625802238
@jujoramos Im not a commiter, so I cannot merge it :)
----------------------------------------------------------------
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] alb3rtobr commented on issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-617861442
> One additional comment regarding the following waning message:
>
> ```
> [warn 2020/04/18 23:44:22.757 PDT <ServerConnection on port 29019 Thread 1> tid=0x298] Unable to ping non-member rs-FullRegression19040559a2i32xlarge-hydra-client-63(bridgegemfire1_host1_4749:4749)<ec><v39>:41003 for client identity(rs-FullRegression19040559a2i32xlarge-hydra-client-63(edgegemfire3_host1_1071:1071:loner):50046:5a182991:edgegemfire3_host1_1071,connection=2
> ```
>
> The above is logged by member `bridgegemfire1_host1_4749` in the tests, exactly the member to which the `ping` command was sent... this warning is logged within the `pingCorrectServer` method which, unless I'm missing something, shouldn't be invoked at all if this member is the actual recipient for the `ping` message. Maybe `!myID.equals(targetServer)` is not working as expected?.
I have added a log right after the comparision to check which values were considered different. I have created two clusters with two servers, and in my case I dont see the comparision is not working, this is an example:
```
[info 2020/04/22 15:05:01.338 GMT <ServerConnection on port 32000 Thread 12> tid=0x56] ALBERTO - MyID=172.17.0.3(server-0:85)<v1>:41000 - target=172.17.0.9(server-1:84)<v2>:41000
```
Could you add the same log to your code?
----------------------------------------------------------------
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 a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r414152050
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -65,8 +71,9 @@ protected boolean needsUserId() {
@Override
protected void sendMessage(Connection cnx) throws Exception {
getMessage().clearMessageHasSecurePartFlag();
- this.startTime = System.currentTimeMillis();
- getMessage().send(false);
+ getMessage().setNumberOfParts(1);
+ getMessage().addObjPart(serverID);
+ getMessage().send(true);
Review comment:
Ops that configure their message in constructors use AbstractOp.sendMessage(). This class already overrode that method so there's no reason to configure the message in the constructor. That could be done though - is it a problem?
Maybe we don't have to clear the message in Message.send(boolean) but the default implementation, Message.send(), does this and since we've introduced a Part to this message I thought it prudent to clear it.
----------------------------------------------------------------
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] onichols-pivotal commented on pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
onichols-pivotal commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-620893200
looks like this PR needs to be rebased to latest develop to fix merge confict with DataSerializableFixedID.java, until then PR checks cannot run.
----------------------------------------------------------------
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] jujoramos commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-621758779
Hello @alb3rtobr
Apparently yes, can you rebase the `PR` against the latest `develop` branch and `push` the changes again?.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418576837
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/PingTest.java
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.tier.sockets.command;
+
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.tier.sockets.Message;
+import org.apache.geode.internal.cache.tier.sockets.Part;
+import org.apache.geode.internal.cache.tier.sockets.ServerConnection;
+
+public class PingTest {
Review comment:
Im not sure if it can be done. The `ClientHealthMonitor` object is obtained using a static method, which cannot be mocked.
----------------------------------------------------------------
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] jujoramos commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-625331128
@alb3rtobr
Thanks for your patience on this. I'll go through the `PR` once again later today or tomorrow morning, I'm running the last battery of tests to double check everything's right :-).
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r413131013
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -43,12 +45,16 @@ private PingOp() {
static class PingOpImpl extends AbstractOp {
private long startTime;
+ private ServerLocation location;
Review comment:
You are right, it seems they are not used, I will remove them.
----------------------------------------------------------------
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] jujoramos commented on issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-617642403
@bschuchardt @alb3rtobr : I'm still analysing the issue and seeing whether I can reproduce it consistently, so far it's proving quite elusive as it only fails once or twice in around 100 runs of my test. Will keep you posted.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r420143599
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -440,6 +460,24 @@ private void removeFromMap(Map<String, Map<ServerLocation, LoadHolder>> map, Str
groupMap.remove(location);
}
+ private void removeFromMap(Map<String, Map<ServerLocationAndMemberId, LoadHolder>> map,
Review comment:
done
----------------------------------------------------------------
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] alb3rtobr commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-621756941
@jujoramos Is there a problem with the CI? Tests have not been executed in the last four commits.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r414535620
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Ping.java
##########
@@ -50,11 +51,17 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
}
if (clientMessage.getNumberOfParts() > 0) {
try {
- DistributedMember targetServer = (DistributedMember) clientMessage.getPart(0).getObject();
- DistributedMember myID = serverConnection.getCache().getMyId();
+ InternalDistributedMember targetServer =
+ (InternalDistributedMember) clientMessage.getPart(0).getObject();
+ InternalDistributedMember myID = serverConnection.getCache().getMyId();
if (!myID.equals(targetServer)) {
- pingCorrectServer(clientMessage, targetServer, serverConnection);
- writeReply(clientMessage, serverConnection);
+ if (myID.compareTo(targetServer.getMemberIdentifier(), true, false) == 0) {
Review comment:
The issue described [here](https://issues.apache.org/jira/browse/GEODE-8004?focusedCommentId=17090468&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-17090468) still remains, you're comparing `myID` (instance of `InternalDistributedMember`) against `targetServer.getMemberIdentifier()` (instance of `MemberIdentifier`), so the comparison fails with a `ClassCastException` and the client logs the following:
```
[warn 2020/04/24 01:33:02.869 PDT <poolTimer-edgeDescript-29> tid=0x112] Pool unexpected java.lang.ClassCastException: [B cannot be cast to java.lang.Throwable connection=Pooled Connection to rs-GEM-2885-0120a0i32xlarge-hydra-client-4:20245: Connection[DESTROYED]). Server unreachable: could not connect after 1 attempts
```
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/membership/InternalDistributedMember.java
##########
@@ -641,4 +641,8 @@ public UUID getUUID() {
public interface HostnameResolver {
InetAddress getInetAddress(ServerLocation location) throws UnknownHostException;
}
+
+ public MemberIdentifier getMemberIdentifier() {
+ return memberIdentifier;
+ }
Review comment:
You don't need to expose the `MemberIdentifier` here, you can directly use `InternalDistributedMember.compareTo(DistributedMember o, boolean compareMemberData, boolean compareViewIds)`, which internally delegates to the `MemberIdentifier` class.
----------------------------------------------------------------
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] alb3rtobr commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-624083268
> Hello @alb3rtobr
> It's looking good!!, some last small requests from my side, and I'll be ready to approve the PR afterwards. @bschuchardt: can you also have another look and make sure I didn't miss anything?.
> Cheers.
I have added all the requested tests :)
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r420143725
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/LocatorLoadSnapshot.java
##########
@@ -425,6 +429,22 @@ private void addGroups(Map<String, Map<ServerLocation, LoadHolder>> map, String[
}
}
+ private void addGroups(Map<String, Map<ServerLocationAndMemberId, LoadHolder>> map,
Review comment:
done
----------------------------------------------------------------
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] alb3rtobr commented on issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-618452679
> @alb3rtobr
> I've been discussing the issue with @bschuchardt and we think you can use `MemberIdentifier.compareTo(other, compareMemberData, compareViewIDs)` to execute the member comparison instead of `equals`, that would guarantee that the `ping` is handled by the current member when the only difference between `myID` and `targetServer` is the `viewId`.
> Sounds feasible?.
I was waiting for Bruce comments after reading your detailed analysis in the jira ticket (thanks for that), and I already had that method in mind, so it sounds fine for me.
----------------------------------------------------------------
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] alb3rtobr commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418558823
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -65,17 +71,33 @@ protected boolean needsUserId() {
@Override
protected void sendMessage(Connection cnx) throws Exception {
getMessage().clearMessageHasSecurePartFlag();
- this.startTime = System.currentTimeMillis();
- getMessage().send(false);
+ getMessage().setNumberOfParts(1);
+ getMessage().addObjPart(serverID);
+ getMessage().send(true);
Message.MESSAGE_TYPE.set(MessageType.PING);
}
@Override
protected Object processResponse(Message msg) throws Exception {
- processAck(msg, "ping");
+ processAck(msg);
return null;
}
+ private void processAck(Message msg) throws Exception {
+ final int msgType = msg.getMessageType();
+ if (msgType != MessageType.REPLY) {
+ Part part = msg.getPart(0);
+ if (msgType == MessageType.EXCEPTION) {
+ Throwable t = (Throwable) part.getObject();
+ throw new ServerOperationException("While performing a remote ping: " + t.getMessage(),
+ t);
+ } else {
+ throw new InternalGemFireError(
+ "Unexpected message type " + MessageType.getString(msgType));
+ }
+ }
+ }
Review comment:
This method is not necessary. I started with a different method, but at the end, its doing the same as `AbstractOp.processAck()`, I will remove it.
----------------------------------------------------------------
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] jujoramos commented on issue #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on issue #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-617866275
@alb3rtobr: Yes, I've added several logging messages to trace the `PING` message from start to finish, unfortunately I've been having some infrastructure issues the whole day and wasn't able to run the tests again with the logging changes. Hopefully will have something to share tomorrow.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418501644
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/PingOpDistributedTest.java
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.tier.sockets;
+
+import static java.util.Arrays.asList;
+import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPortsForDUnitSite;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.PoolFactory;
+import org.apache.geode.cache.client.PoolManager;
+import org.apache.geode.cache.client.ServerOperationException;
+import org.apache.geode.cache.client.internal.PingOp;
+import org.apache.geode.cache.client.internal.PoolImpl;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.ClientCacheRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+import org.apache.geode.test.junit.rules.serializable.SerializableTemporaryFolder;
+import org.apache.geode.test.junit.rules.serializable.SerializableTestName;
+
+public class PingOpDistributedTest implements Serializable {
+
+ @Rule
+ public CacheRule cacheRule = new CacheRule();
+
+ @Rule
+ public ClientCacheRule clientCacheRule = new ClientCacheRule();
+
+ @Rule
+ public SerializableTestName testName = new SerializableTestName();
+
+ @Rule
+ public DistributedRule distributedRule = new DistributedRule(2);
+
+ @Rule
+ public SerializableTemporaryFolder folder = new SerializableTemporaryFolder();
+
+ private VM client;
+ private VM server1, server2;
+ private int server1Port, server2Port;
+
+
+ private void initServer(int serverPort) throws IOException {
+ cacheRule.createCache();
+ CacheServer cacheServer = cacheRule.getCache().addCacheServer();
+ cacheServer.setPort(serverPort);
+
+ // "Disable" the auto-ping for the duration of this test.
+ cacheServer.setMaximumTimeBetweenPings((int) GeodeAwaitility.getTimeout().toMillis());
+ cacheServer.start();
+ }
+
+ private void initClient(String poolName, List<Integer> serverPorts) {
+ final ClientCacheFactory clientCacheFactory = new ClientCacheFactory();
+ clientCacheFactory.create();
+
+ PoolFactory poolFactory = PoolManager.createFactory();
+ serverPorts.forEach(serverPort -> poolFactory.addServer("localhost", serverPort));
+
+ // "Disable" the auto-ping for the duration of this test.
+ poolFactory.setPingInterval((int) GeodeAwaitility.getTimeout().toMillis());
+ poolFactory.create(poolName);
+ }
+
+ @Before
+ public void setUp() throws IOException {
+ int[] ports = getRandomAvailableTCPPortsForDUnitSite(2);
+
+ client = getVM(0);
+ server1 = getVM(1);
+ server2 = getVM(2);
+ server1Port = ports[0];
+ server2Port = ports[1];
+ server1.invoke(() -> initServer(server1Port));
+ server2.invoke(() -> initServer(server2Port));
+ }
+
+ void parametrizedSetUp(String poolName, List<Integer> serverPorts) {
+ client.invoke(() -> initClient(poolName, serverPorts));
+ }
+
+ public void executePing(String poolName, int serverPort,
+ InternalDistributedMember distributedMember) {
+ PoolImpl poolImpl = (PoolImpl) PoolManager.find(poolName);
+ PingOp.execute(poolImpl, new ServerLocation("localhost", serverPort), distributedMember);
+ }
+
+ public Long getSingleHeartBeat() {
+ ClientHealthMonitor chm = ClientHealthMonitor.getInstance();
+ if (chm.getClientHeartbeats().size() == 0) {
+ return 0L;
+ }
+ assertThat(chm.getClientHeartbeats()).isNotEmpty().hasSize(1);
+
+ return chm.getClientHeartbeats().entrySet().iterator().next().getValue();
+ }
+
+ @Test
+ public void regularPingFlow() {
+ final String poolName = testName.getMethodName();
+ parametrizedSetUp(poolName, Collections.singletonList(server1Port));
+ InternalDistributedMember distributedMember1 = (InternalDistributedMember) server1
+ .invoke(() -> cacheRule.getCache().getDistributedSystem().getDistributedMember());
+
+ client.invoke(() -> executePing(poolName, server1Port, distributedMember1));
+ Long firstHeartbeat = server1.invoke(this::getSingleHeartBeat);
+
+ client.invoke(() -> executePing(poolName, server1Port, distributedMember1));
+ Long secondHeartbeat = server1.invoke(this::getSingleHeartBeat);
+
+ assertThat(secondHeartbeat).isGreaterThan(firstHeartbeat);
+
+ }
+
+ @Test
+ public void memberShouldNotRedirectPingMessageWhenClientCachedViewIdIsWrong() throws IOException {
Review comment:
The exception is not thrown anywhere in the method, it can be removed.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/PingOpDistributedTest.java
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.tier.sockets;
+
+import static java.util.Arrays.asList;
+import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPortsForDUnitSite;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.PoolFactory;
+import org.apache.geode.cache.client.PoolManager;
+import org.apache.geode.cache.client.ServerOperationException;
+import org.apache.geode.cache.client.internal.PingOp;
+import org.apache.geode.cache.client.internal.PoolImpl;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.ClientCacheRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+import org.apache.geode.test.junit.rules.serializable.SerializableTemporaryFolder;
+import org.apache.geode.test.junit.rules.serializable.SerializableTestName;
+
+public class PingOpDistributedTest implements Serializable {
+
+ @Rule
+ public CacheRule cacheRule = new CacheRule();
+
+ @Rule
+ public ClientCacheRule clientCacheRule = new ClientCacheRule();
+
+ @Rule
+ public SerializableTestName testName = new SerializableTestName();
+
+ @Rule
+ public DistributedRule distributedRule = new DistributedRule(2);
+
+ @Rule
+ public SerializableTemporaryFolder folder = new SerializableTemporaryFolder();
+
+ private VM client;
+ private VM server1, server2;
+ private int server1Port, server2Port;
+
+
+ private void initServer(int serverPort) throws IOException {
+ cacheRule.createCache();
+ CacheServer cacheServer = cacheRule.getCache().addCacheServer();
+ cacheServer.setPort(serverPort);
+
+ // "Disable" the auto-ping for the duration of this test.
+ cacheServer.setMaximumTimeBetweenPings((int) GeodeAwaitility.getTimeout().toMillis());
+ cacheServer.start();
+ }
+
+ private void initClient(String poolName, List<Integer> serverPorts) {
+ final ClientCacheFactory clientCacheFactory = new ClientCacheFactory();
+ clientCacheFactory.create();
+
+ PoolFactory poolFactory = PoolManager.createFactory();
+ serverPorts.forEach(serverPort -> poolFactory.addServer("localhost", serverPort));
+
+ // "Disable" the auto-ping for the duration of this test.
+ poolFactory.setPingInterval((int) GeodeAwaitility.getTimeout().toMillis());
+ poolFactory.create(poolName);
+ }
+
+ @Before
+ public void setUp() throws IOException {
+ int[] ports = getRandomAvailableTCPPortsForDUnitSite(2);
+
+ client = getVM(0);
+ server1 = getVM(1);
+ server2 = getVM(2);
+ server1Port = ports[0];
+ server2Port = ports[1];
+ server1.invoke(() -> initServer(server1Port));
+ server2.invoke(() -> initServer(server2Port));
+ }
+
+ void parametrizedSetUp(String poolName, List<Integer> serverPorts) {
+ client.invoke(() -> initClient(poolName, serverPorts));
+ }
+
+ public void executePing(String poolName, int serverPort,
+ InternalDistributedMember distributedMember) {
+ PoolImpl poolImpl = (PoolImpl) PoolManager.find(poolName);
+ PingOp.execute(poolImpl, new ServerLocation("localhost", serverPort), distributedMember);
+ }
+
+ public Long getSingleHeartBeat() {
+ ClientHealthMonitor chm = ClientHealthMonitor.getInstance();
+ if (chm.getClientHeartbeats().size() == 0) {
+ return 0L;
+ }
+ assertThat(chm.getClientHeartbeats()).isNotEmpty().hasSize(1);
+
+ return chm.getClientHeartbeats().entrySet().iterator().next().getValue();
+ }
+
+ @Test
+ public void regularPingFlow() {
+ final String poolName = testName.getMethodName();
+ parametrizedSetUp(poolName, Collections.singletonList(server1Port));
+ InternalDistributedMember distributedMember1 = (InternalDistributedMember) server1
+ .invoke(() -> cacheRule.getCache().getDistributedSystem().getDistributedMember());
+
+ client.invoke(() -> executePing(poolName, server1Port, distributedMember1));
+ Long firstHeartbeat = server1.invoke(this::getSingleHeartBeat);
+
+ client.invoke(() -> executePing(poolName, server1Port, distributedMember1));
+ Long secondHeartbeat = server1.invoke(this::getSingleHeartBeat);
+
+ assertThat(secondHeartbeat).isGreaterThan(firstHeartbeat);
+
+ }
+
+ @Test
+ public void memberShouldNotRedirectPingMessageWhenClientCachedViewIdIsWrong() throws IOException {
+ final String poolName = testName.getMethodName();
+ parametrizedSetUp(poolName, Collections.singletonList(server1Port));
+ InternalDistributedMember distributedMember1 = (InternalDistributedMember) server1
+ .invoke(() -> cacheRule.getCache().getDistributedSystem().getDistributedMember());
+
+ client.invoke(() -> {
+ PoolImpl poolImpl = (PoolImpl) PoolManager.find(poolName);
+ distributedMember1.setVmViewId(distributedMember1.getVmViewId() + 1);
+ assertThatThrownBy(() -> {
+ PingOp.execute(poolImpl, new ServerLocation("localhost", server1Port), distributedMember1);
+ }).isInstanceOf(ServerOperationException.class).hasMessageContaining("has different viewId:");
+ });
+ }
+
+ @Test
+ public void pingReturnsErrorIfTheTargetServerIsNotAMember() throws IOException {
Review comment:
The exception is not thrown anywhere in the method, it can be removed.
##########
File path: geode-core/src/distributedTest/java/org/apache/geode/internal/cache/tier/sockets/PingOpDistributedTest.java
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.tier.sockets;
+
+import static java.util.Arrays.asList;
+import static org.apache.geode.internal.AvailablePortHelper.getRandomAvailableTCPPortsForDUnitSite;
+import static org.apache.geode.test.dunit.VM.getVM;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import java.io.IOException;
+import java.io.Serializable;
+import java.util.Collections;
+import java.util.List;
+
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+
+import org.apache.geode.cache.client.ClientCacheFactory;
+import org.apache.geode.cache.client.PoolFactory;
+import org.apache.geode.cache.client.PoolManager;
+import org.apache.geode.cache.client.ServerOperationException;
+import org.apache.geode.cache.client.internal.PingOp;
+import org.apache.geode.cache.client.internal.PoolImpl;
+import org.apache.geode.cache.server.CacheServer;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.test.awaitility.GeodeAwaitility;
+import org.apache.geode.test.dunit.VM;
+import org.apache.geode.test.dunit.rules.CacheRule;
+import org.apache.geode.test.dunit.rules.ClientCacheRule;
+import org.apache.geode.test.dunit.rules.DistributedRule;
+import org.apache.geode.test.junit.rules.serializable.SerializableTemporaryFolder;
+import org.apache.geode.test.junit.rules.serializable.SerializableTestName;
+
+public class PingOpDistributedTest implements Serializable {
+
+ @Rule
+ public CacheRule cacheRule = new CacheRule();
+
+ @Rule
+ public ClientCacheRule clientCacheRule = new ClientCacheRule();
+
+ @Rule
+ public SerializableTestName testName = new SerializableTestName();
+
+ @Rule
+ public DistributedRule distributedRule = new DistributedRule(2);
+
+ @Rule
+ public SerializableTemporaryFolder folder = new SerializableTemporaryFolder();
+
+ private VM client;
+ private VM server1, server2;
+ private int server1Port, server2Port;
+
+
+ private void initServer(int serverPort) throws IOException {
+ cacheRule.createCache();
+ CacheServer cacheServer = cacheRule.getCache().addCacheServer();
+ cacheServer.setPort(serverPort);
+
+ // "Disable" the auto-ping for the duration of this test.
+ cacheServer.setMaximumTimeBetweenPings((int) GeodeAwaitility.getTimeout().toMillis());
+ cacheServer.start();
+ }
+
+ private void initClient(String poolName, List<Integer> serverPorts) {
+ final ClientCacheFactory clientCacheFactory = new ClientCacheFactory();
+ clientCacheFactory.create();
+
+ PoolFactory poolFactory = PoolManager.createFactory();
+ serverPorts.forEach(serverPort -> poolFactory.addServer("localhost", serverPort));
+
+ // "Disable" the auto-ping for the duration of this test.
+ poolFactory.setPingInterval((int) GeodeAwaitility.getTimeout().toMillis());
+ poolFactory.create(poolName);
+ }
+
+ @Before
+ public void setUp() throws IOException {
+ int[] ports = getRandomAvailableTCPPortsForDUnitSite(2);
+
+ client = getVM(0);
+ server1 = getVM(1);
+ server2 = getVM(2);
+ server1Port = ports[0];
+ server2Port = ports[1];
+ server1.invoke(() -> initServer(server1Port));
+ server2.invoke(() -> initServer(server2Port));
+ }
+
+ void parametrizedSetUp(String poolName, List<Integer> serverPorts) {
+ client.invoke(() -> initClient(poolName, serverPorts));
+ }
+
+ public void executePing(String poolName, int serverPort,
+ InternalDistributedMember distributedMember) {
+ PoolImpl poolImpl = (PoolImpl) PoolManager.find(poolName);
+ PingOp.execute(poolImpl, new ServerLocation("localhost", serverPort), distributedMember);
+ }
+
+ public Long getSingleHeartBeat() {
+ ClientHealthMonitor chm = ClientHealthMonitor.getInstance();
+ if (chm.getClientHeartbeats().size() == 0) {
+ return 0L;
+ }
+ assertThat(chm.getClientHeartbeats()).isNotEmpty().hasSize(1);
+
+ return chm.getClientHeartbeats().entrySet().iterator().next().getValue();
+ }
+
+ @Test
+ public void regularPingFlow() {
+ final String poolName = testName.getMethodName();
+ parametrizedSetUp(poolName, Collections.singletonList(server1Port));
+ InternalDistributedMember distributedMember1 = (InternalDistributedMember) server1
+ .invoke(() -> cacheRule.getCache().getDistributedSystem().getDistributedMember());
+
+ client.invoke(() -> executePing(poolName, server1Port, distributedMember1));
+ Long firstHeartbeat = server1.invoke(this::getSingleHeartBeat);
+
+ client.invoke(() -> executePing(poolName, server1Port, distributedMember1));
+ Long secondHeartbeat = server1.invoke(this::getSingleHeartBeat);
+
+ assertThat(secondHeartbeat).isGreaterThan(firstHeartbeat);
+
+ }
+
+ @Test
+ public void memberShouldNotRedirectPingMessageWhenClientCachedViewIdIsWrong() throws IOException {
+ final String poolName = testName.getMethodName();
+ parametrizedSetUp(poolName, Collections.singletonList(server1Port));
+ InternalDistributedMember distributedMember1 = (InternalDistributedMember) server1
+ .invoke(() -> cacheRule.getCache().getDistributedSystem().getDistributedMember());
+
+ client.invoke(() -> {
+ PoolImpl poolImpl = (PoolImpl) PoolManager.find(poolName);
+ distributedMember1.setVmViewId(distributedMember1.getVmViewId() + 1);
+ assertThatThrownBy(() -> {
+ PingOp.execute(poolImpl, new ServerLocation("localhost", server1Port), distributedMember1);
+ }).isInstanceOf(ServerOperationException.class).hasMessageContaining("has different viewId:");
+ });
+ }
+
+ @Test
+ public void pingReturnsErrorIfTheTargetServerIsNotAMember() throws IOException {
+ final String poolName = testName.getMethodName();
+ parametrizedSetUp(poolName, Collections.singletonList(server1Port));
+ int notUsedPort = getRandomAvailableTCPPortsForDUnitSite(1)[0];
+ InternalDistributedMember fakeDistributedMember =
+ new InternalDistributedMember("localhost", notUsedPort);
+ client.invoke(() -> {
+ PoolImpl poolImpl = (PoolImpl) PoolManager.find(poolName);
+ assertThatThrownBy(() -> {
+ PingOp.execute(poolImpl, new ServerLocation("localhost", server1Port),
+ fakeDistributedMember);
+ }).isInstanceOf(ServerOperationException.class)
+ .hasMessageContaining("Unable to ping non-member");
+ });
+ }
+
+ @Test
+ public void memberShouldCorrectlyRedirectPingMessage() throws IOException, InterruptedException {
Review comment:
The exceptions are not thrown anywhere in the method, they can be removed.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/tier/sockets/command/Ping.java
##########
@@ -47,10 +50,37 @@ public void cmdExecute(final Message clientMessage, final ServerConnection serve
clientMessage.getTransactionId(), serverConnection.getSocketString(),
(DistributionStats.getStatTime() - start));
}
+ if (clientMessage.getNumberOfParts() > 0) {
+ try {
+ InternalDistributedMember targetServer =
+ (InternalDistributedMember) clientMessage.getPart(0).getObject();
+ InternalDistributedMember myID = serverConnection.getCache().getMyId();
+ if (!myID.equals(targetServer)) {
+ if (myID.compareTo(targetServer, true, false) == 0) {
+ String errorMessage =
+ String.format("Target server " + targetServer + " has different viewId: " + myID);
Review comment:
No need to use `format` as we are not using parameters, just concatenating `String` objects.
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ServerLocationAndMemberId.java
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.distributed.internal;
+
+public class ServerLocationAndMemberId {
+
+ private ServerLocation serverLocation;
+ private String memberId;
Review comment:
Can these two fields be declared as `final`?.
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -65,17 +71,33 @@ protected boolean needsUserId() {
@Override
protected void sendMessage(Connection cnx) throws Exception {
getMessage().clearMessageHasSecurePartFlag();
- this.startTime = System.currentTimeMillis();
- getMessage().send(false);
+ getMessage().setNumberOfParts(1);
+ getMessage().addObjPart(serverID);
+ getMessage().send(true);
Message.MESSAGE_TYPE.set(MessageType.PING);
}
@Override
protected Object processResponse(Message msg) throws Exception {
- processAck(msg, "ping");
+ processAck(msg);
return null;
}
+ private void processAck(Message msg) throws Exception {
+ final int msgType = msg.getMessageType();
+ if (msgType != MessageType.REPLY) {
+ Part part = msg.getPart(0);
+ if (msgType == MessageType.EXCEPTION) {
+ Throwable t = (Throwable) part.getObject();
+ throw new ServerOperationException("While performing a remote ping: " + t.getMessage(),
+ t);
+ } else {
+ throw new InternalGemFireError(
+ "Unexpected message type " + MessageType.getString(msgType));
+ }
+ }
+ }
Review comment:
I'm not sure it is entirely correct to override the `procesAck` method... Looking through the source code, there's only one operation overriding this (`TXSynchronizationOp`) and it ends up delegating to the super class method whenever the exception is not the one it knows how to handle. There's also a comment regarding `c++` clients in `AbstractOp.processAck()`, we should be careful not to break those clients as well.
@bschuchardt: thoughts on this one?.
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/GridAdvisor.java
##########
@@ -418,18 +418,24 @@ public String toString() {
public int hashCode() {
final String thisHost = this.gp.getHost();
final int thisPort = this.gp.getPort();
- return thisHost != null ? (thisHost.hashCode() ^ thisPort) : thisPort;
+ final String thisMemberId = this.getMemberId().getUniqueId();
+ final int thisMemberIdHashCode = (thisMemberId != null) ? thisMemberId.hashCode() : 0;
+ return thisHost != null ? (thisHost.hashCode() ^ thisPort) + thisMemberIdHashCode
+ : thisPort + thisMemberIdHashCode;
}
@Override
public boolean equals(Object obj) {
if (obj instanceof GridProfileId) {
final GridProfileId other = (GridProfileId) obj;
+
if (this.gp.getPort() == other.gp.getPort()) {
final String thisHost = this.gp.getHost();
final String otherHost = other.gp.getHost();
if (thisHost != null) {
- return thisHost.equals(otherHost);
+ if (thisHost.equals(otherHost)) {
+ return this.getMemberId().equals(other.getMemberId());
Review comment:
Should we use `getMemberId().getUniqueId()` here instead of just `getMemberId()`, as that's what we are using to calculate the `hashCode`?.
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/PingTest.java
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.tier.sockets.command;
+
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.tier.sockets.Message;
+import org.apache.geode.internal.cache.tier.sockets.Part;
+import org.apache.geode.internal.cache.tier.sockets.ServerConnection;
+
+public class PingTest {
Review comment:
Tests could be improved to also assert that the receivedPing method from the ClientHealthMonitor class has been invoked when required. Just a suggestion, anyways.
----------------------------------------------------------------
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] jujoramos commented on pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-619040254
> I hope last two commits solve the failure I introduced, sorry for that.
Thanks, I need to have a look and run the tests again; will let you know the results early next week.
----------------------------------------------------------------
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] alb3rtobr commented on pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
alb3rtobr commented on pull request #4978:
URL: https://github.com/apache/geode/pull/4978#issuecomment-625761103
Thanks @jujoramos & @bschuchardt ! Is there anything else missing to merge and close the 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] bschuchardt commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
bschuchardt commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r420211074
##########
File path: geode-core/src/main/java/org/apache/geode/distributed/internal/ServerLocationAndMemberId.java
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.distributed.internal;
+
+public class ServerLocationAndMemberId {
+
+ private final ServerLocation serverLocation;
+ private final String memberId;
+
+ public ServerLocationAndMemberId(ServerLocation serverLocation, String memberId) {
+ this.serverLocation = serverLocation;
+ this.memberId = memberId;
+ }
+
+ public ServerLocation getServerLocation() {
+ return this.serverLocation;
+ }
+
+ public String getMemberId() {
+ return this.memberId;
+ }
+
+ @Override
+ public boolean equals(Object obj) {
+ if (this == obj)
+ return true;
+ if (obj == null)
+ return false;
+ if (!(obj instanceof ServerLocationAndMemberId))
+ return false;
+ final ServerLocationAndMemberId other = (ServerLocationAndMemberId) obj;
+
+ if (!this.serverLocation.equals(other.getServerLocation())) {
+ return false;
+ }
+
+ return this.memberId.equals(other.getMemberId());
Review comment:
There is a null check for memberId in hashCode() but not in equals(). If it's possible for memberId to be null then you should add a null check to equals().
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/GridAdvisor.java
##########
@@ -418,18 +418,24 @@ public String toString() {
public int hashCode() {
final String thisHost = this.gp.getHost();
final int thisPort = this.gp.getPort();
- return thisHost != null ? (thisHost.hashCode() ^ thisPort) : thisPort;
+ final String thisMemberId = this.getMemberId().getUniqueId();
+ final int thisMemberIdHashCode = (thisMemberId != null) ? thisMemberId.hashCode() : 0;
+ return thisHost != null ? (thisHost.hashCode() ^ thisPort) + thisMemberIdHashCode
+ : thisPort + thisMemberIdHashCode;
}
@Override
public boolean equals(Object obj) {
if (obj instanceof GridProfileId) {
final GridProfileId other = (GridProfileId) obj;
+
if (this.gp.getPort() == other.gp.getPort()) {
final String thisHost = this.gp.getHost();
final String otherHost = other.gp.getHost();
if (thisHost != null) {
- return thisHost.equals(otherHost);
+ if (thisHost.equals(otherHost)) {
+ return this.getMemberId().getUniqueId().equals(other.getMemberId().getUniqueId());
Review comment:
This pair of equals()/hashCode() methods has the same problem.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r414385715
##########
File path: geode-core/src/main/java/org/apache/geode/cache/client/internal/PingOp.java
##########
@@ -65,8 +71,9 @@ protected boolean needsUserId() {
@Override
protected void sendMessage(Connection cnx) throws Exception {
getMessage().clearMessageHasSecurePartFlag();
- this.startTime = System.currentTimeMillis();
- getMessage().send(false);
+ getMessage().setNumberOfParts(1);
+ getMessage().addObjPart(serverID);
+ getMessage().send(true);
Review comment:
No problem at all, I was just checking as I saw other messages execute the entire configuration logic (set number of parts, adding parts, etc.) directly in the constructor.
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418051895
##########
File path: geode-core/src/main/java/org/apache/geode/internal/cache/DistributedPingMessage.java
##########
@@ -0,0 +1,79 @@
+/*
+ * 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;
+
+import java.io.DataInput;
+import java.io.DataOutput;
+import java.io.IOException;
+
+import org.apache.geode.distributed.DistributedMember;
+import org.apache.geode.distributed.internal.ClusterDistributionManager;
+import org.apache.geode.distributed.internal.HighPriorityDistributionMessage;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.tier.sockets.ClientHealthMonitor;
+import org.apache.geode.internal.cache.tier.sockets.ClientProxyMembershipID;
+import org.apache.geode.internal.serialization.DeserializationContext;
+import org.apache.geode.internal.serialization.SerializationContext;
+import org.apache.geode.internal.serialization.Version;
+
+public class DistributedPingMessage extends HighPriorityDistributionMessage {
Review comment:
Hey @alb3rtobr
Agreed, the class itself is pretty simple... Having extra unit tests is harmless (and beneficial), however, so I'd say go ahead and add them, it shouldn't long :-).
----------------------------------------------------------------
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] jujoramos commented on a change in pull request #4978: GEODE-8004: Fix for regression introduced by GEODE-7565
Posted by GitBox <gi...@apache.org>.
jujoramos commented on a change in pull request #4978:
URL: https://github.com/apache/geode/pull/4978#discussion_r418513427
##########
File path: geode-core/src/test/java/org/apache/geode/internal/cache/tier/sockets/command/PingTest.java
##########
@@ -0,0 +1,136 @@
+/*
+ * 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.tier.sockets.command;
+
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import org.apache.geode.CancelCriterion;
+import org.apache.geode.distributed.internal.DistributionManager;
+import org.apache.geode.distributed.internal.ServerLocation;
+import org.apache.geode.distributed.internal.membership.InternalDistributedMember;
+import org.apache.geode.internal.cache.InternalCache;
+import org.apache.geode.internal.cache.tier.sockets.Message;
+import org.apache.geode.internal.cache.tier.sockets.Part;
+import org.apache.geode.internal.cache.tier.sockets.ServerConnection;
+
+public class PingTest {
Review comment:
Tests could be improved to also assert that the `receivedPing` method from the `ClientHealthMonitor` class has been invoked when required. Just a suggestion, anyways.
----------------------------------------------------------------
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