upthewaterspout commented on a change in pull request #7113:
URL: https://github.com/apache/geode/pull/7113#discussion_r827256779
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecoverer.java
##########
@@ -197,17 +209,23 @@ public void run2() {
// Log and bail
logger.error(e.getMessage(), e);
} finally {
- /*
- * Our job is done. Stop listening to the bucket advisors.
- */
- removeListeners();
- /*
- * Make sure the recovery completion message was printed to the log.
- */
- for (RegionStatus region : regions) {
- if (!region.loggedDoneMessage) {
- region.logDoneMessage();
+ if (!regions.isEmpty()) {
+
+ /*
+ * Our job is done. Stop listening to the bucket advisors.
+ */
+ removeListeners();
+ /*
+ * Make sure the recovery completion message was printed to the log.
+ */
+
+ for (RegionStatus region : regions) {
+ if (!region.loggedDoneMessage) {
+ region.logDoneMessage();
+ }
}
+
+ regions.clear();
Review comment:
I don't think this clear should be necessary either - once this thread
exits this whole object should be garbage collected.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecoverer.java
##########
@@ -197,17 +209,23 @@ public void run2() {
// Log and bail
logger.error(e.getMessage(), e);
} finally {
- /*
- * Our job is done. Stop listening to the bucket advisors.
- */
- removeListeners();
- /*
- * Make sure the recovery completion message was printed to the log.
- */
- for (RegionStatus region : regions) {
- if (!region.loggedDoneMessage) {
- region.logDoneMessage();
+ if (!regions.isEmpty()) {
Review comment:
I'm not sure this if check is necessary. Everything inside here will
work if regions is empty, and will have no effect if regions is not.
##########
File path:
geode-core/src/main/java/org/apache/geode/internal/cache/partitioned/PersistentBucketRecoverer.java
##########
@@ -336,6 +361,9 @@ private PersistentMemberID
createPersistentMemberID(PartitionedRegion region) {
}
}
}
+ if (allBucketsClosed) {
Review comment:
I wonder if this could be simpler if we just checked the status of the
whole region, rather than looking at all of the proxy bucket regions and only
throwing an exception if they are all closed.
I also wonder if we are going to see some strange behavior if the region is
in the middle of shutting down and has only closed some of the proxy bucket
regions? I guess since this is a log message, if we are just missing some of
the buckets in the message at the same time the region is shutting down maybe
we don't care...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
