[GitHub] [geode] jchen21 commented on a diff in pull request #7670: [Don't review]: GEODE-10290: GII requester should remove departed members

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

jchen21 commented on code in PR #7670:
URL: https://github.com/apache/geode/pull/7670#discussion_r869675129


##########
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java:
##########
@@ -1077,7 +1081,8 @@ protected Set<Object> processReceivedRVV(RegionVersionVector remoteRVV,
         if (id == null) {
           id = myId;
         }
-        if (!remoteRVV.contains(id, stamp.getRegionVersion())) {
+        foundIds.add(id);

Review Comment:
   This will add `id` to `foundIds`, even for the persistent case. However, according to the description of GEODE-10290, it is for the non-persistent case:
   >In non-persistent but concurrent-check enabled case, members departed will be marked. They should be removed from RVV during GII to prevent memberToVersion in RVV grows bigger and bigger.
   
   So I am not sure if we should add a `if` check here. e.g. `if (!isPersistentRegion)` .



##########
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java:
##########
@@ -1100,6 +1105,12 @@ protected Set<Object> processReceivedRVV(RegionVersionVector remoteRVV,
         }
       }
     }
+    if (foundIds.size() > 0) {

Review Comment:
   `foundIds` always has at least one element. Because of line 1067 `foundIds.add(region.getVersionMember());`.  So this `if ` check is not necessary. 
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@geode.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org