[GitHub] [geode] jujoramos commented on a change in pull request #4978: Fix for regression introduced by GEODE-7565

[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