dschneider-pivotal commented on code in PR #7670:
URL: https://github.com/apache/geode/pull/7670#discussion_r874251073
##########
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java:
##########
@@ -1052,19 +1052,23 @@ protected RegionVersionVector getRVVFromProvider(final
ClusterDistributionManage
/**
* Compare the received RVV with local RVV and return a set of keys for
unfinished operations.
*
- * @param remoteRVV RVV from provider
+ * @param remoteRVV RVV from provider to be filled with unfinished operations
* @param localRVV RVV recovered from disk
+ * @param receivedRVV original RVV from provider to remove departed members
* @return set for keys of unfinished operations.
*/
protected Set<Object> processReceivedRVV(RegionVersionVector remoteRVV,
- RegionVersionVector localRVV) {
+ RegionVersionVector localRVV, RegionVersionVector receivedRVV) {
if (remoteRVV == null) {
return null;
}
// calculate keys for unfinished ops
+ Set<VersionSource> foundIds = new HashSet<>();
Review Comment:
no need to allocate a real HashSet for foundIds if "isPersistentRegion". For
persistent regions this could just be the canonical Collections.emptySet().
##########
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java:
##########
@@ -1100,6 +1107,13 @@ protected Set<Object>
processReceivedRVV(RegionVersionVector remoteRVV,
}
}
}
+ if (!departedMemberSet.isEmpty() && foundIds.size() > 0) {
Review Comment:
instead of size() > 0 use !isEmpty()
also I don't see any need to check departedMemberSet here if you make the
other changes.
##########
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java:
##########
@@ -1052,19 +1052,23 @@ protected RegionVersionVector getRVVFromProvider(final
ClusterDistributionManage
/**
* Compare the received RVV with local RVV and return a set of keys for
unfinished operations.
*
- * @param remoteRVV RVV from provider
+ * @param remoteRVV RVV from provider to be filled with unfinished operations
* @param localRVV RVV recovered from disk
+ * @param receivedRVV original RVV from provider to remove departed members
* @return set for keys of unfinished operations.
*/
protected Set<Object> processReceivedRVV(RegionVersionVector remoteRVV,
- RegionVersionVector localRVV) {
+ RegionVersionVector localRVV, RegionVersionVector receivedRVV) {
if (remoteRVV == null) {
return null;
}
// calculate keys for unfinished ops
+ Set<VersionSource> foundIds = new HashSet<>();
HashSet<Object> keys = new HashSet<>();
- if (region.getDataPolicy().withPersistence()
- && localRVV.isNewerThanOrCanFillExceptionsFor(remoteRVV)) {
+ Set<VersionSource> departedMemberSet = receivedRVV.getDepartedMembersSet();
+ boolean isPersistentRegion = region.getDataPolicy().withPersistence();
+ if ((isPersistentRegion &&
localRVV.isNewerThanOrCanFillExceptionsFor(remoteRVV))
+ || !departedMemberSet.isEmpty()) {
Review Comment:
since foundIds is only added to if '!isPersistentRegion' it seems like this
OR should be "|| (!isPersistentRegion && !departedMemberSet.isEmpty())" .
Otherwise we go into this "if" for a persistentRegion if departedMemberSet
is not empty.
##########
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java:
##########
@@ -1052,19 +1052,23 @@ protected RegionVersionVector getRVVFromProvider(final
ClusterDistributionManage
/**
* Compare the received RVV with local RVV and return a set of keys for
unfinished operations.
*
- * @param remoteRVV RVV from provider
+ * @param remoteRVV RVV from provider to be filled with unfinished operations
* @param localRVV RVV recovered from disk
+ * @param receivedRVV original RVV from provider to remove departed members
* @return set for keys of unfinished operations.
*/
protected Set<Object> processReceivedRVV(RegionVersionVector remoteRVV,
- RegionVersionVector localRVV) {
+ RegionVersionVector localRVV, RegionVersionVector receivedRVV) {
if (remoteRVV == null) {
return null;
}
// calculate keys for unfinished ops
+ Set<VersionSource> foundIds = new HashSet<>();
HashSet<Object> keys = new HashSet<>();
- if (region.getDataPolicy().withPersistence()
- && localRVV.isNewerThanOrCanFillExceptionsFor(remoteRVV)) {
+ Set<VersionSource> departedMemberSet = receivedRVV.getDepartedMembersSet();
+ boolean isPersistentRegion = region.getDataPolicy().withPersistence();
+ if ((isPersistentRegion &&
localRVV.isNewerThanOrCanFillExceptionsFor(remoteRVV))
+ || !departedMemberSet.isEmpty()) {
// only search for unfinished keys when localRVV has something newer
Review Comment:
this comment needs to be updated since you now also do the search for
non-persistent with departedMembers
##########
geode-core/src/main/java/org/apache/geode/internal/cache/InitialImageOperation.java:
##########
@@ -1077,7 +1081,10 @@ protected Set<Object>
processReceivedRVV(RegionVersionVector remoteRVV,
if (id == null) {
id = myId;
}
- if (!remoteRVV.contains(id, stamp.getRegionVersion())) {
+ if (!isPersistentRegion) {
+ foundIds.add(id);
+ }
+ if (isPersistentRegion && !remoteRVV.contains(id,
stamp.getRegionVersion())) {
Review Comment:
instead of testing "isPersistentRegion" in this "if" just add an "else"
before the "if".
--
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-unsubscr...@geode.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
