-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HBASE-27752: Update the block cache and list of prefetched files upon region movement #5194
Conversation
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this change has impacted TestPrefetchRSClose.testPrefetchPersistence?
rsServices.getBlockCache().ifPresent(blockCache -> { | ||
if (blockCache instanceof CombinedBlockCache) { | ||
BlockCache l2 = ((CombinedBlockCache)blockCache).getSecondLevelCache(); | ||
if (l2 instanceof BucketCache) { | ||
if (region.getReadOnlyConfiguration().get(PREFETCH_PERSISTENCE_PATH_KEY) != null) { | ||
LOG.info("Closing region {} during a graceful stop, and prefetch persistence is on, " | ||
+ "so setting evict on close to false. ", region.getRegionInfo().getEncodedName()); | ||
region.getStores().forEach(s -> s.getCacheConfig().setEvictOnClose(false)); | ||
} | ||
} | ||
} | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I've done something similar in UnassignRegionHandler for HBASE-27474, but I wonder now if we should move these logic about evict decision to a related methods in HRegion.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
return close(abort, false, false); | ||
} | ||
|
||
public Map<byte[], List<HStoreFile>> close(boolean abort, boolean isGracefulStop) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there's some confusion with the parameters here. We should keep the original signature public Map<byte[], List<HStoreFile>> close(boolean abort, boolean ignoreStatus)
and pass ignoreStatus
along in the delegating call (here we are always forcing it to false). Could it be behind the latest UT failureS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wchevreuil I have addressed this feedback. However, the previous UTs are probably not failing with this change here. Let's wait for the pre-commit to see how it goes this time.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -1596,6 +1598,11 @@ public Map<byte[], List<HStoreFile>> close(boolean abort) throws IOException { | |||
return close(abort, false); | |||
} | |||
|
|||
public Map<byte[], List<HStoreFile>> close(boolean abort, boolean ignoreStatus) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: javadoc required;
@@ -1610,8 +1617,8 @@ public Map<byte[], List<HStoreFile>> close(boolean abort) throws IOException { | |||
* not properly persisted. The region is put in closing mode, and | |||
* the caller MUST abort after this. | |||
*/ | |||
public Map<byte[], List<HStoreFile>> close(boolean abort, boolean ignoreStatus) | |||
throws IOException { | |||
public Map<byte[], List<HStoreFile>> close(boolean abort, boolean ignoreStatus, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: update javadoc with the additional param.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
@@ -134,7 +134,8 @@ public static void cancel(Path path) { | |||
prefetchFutures.remove(path); | |||
LOG.debug("Prefetch cancelled for {}", path); | |||
} | |||
prefetchCompleted.remove(path.getName()); | |||
LOG.debug("Removing filename from the prefetched persistence list: " + path.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: use parameterised log4j messaging.
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thank you, @wchevreuil |
…apache#5194) Co-authored-by: Shanmukha Kota <skota@cloudera.com> (cherry picked from commit ece8d01)
…apache#5194) Co-authored-by: Shanmukha Kota <skota@cloudera.com> (cherry picked from commit ece8d01)
…#5194) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org>
…ion movement (apache#5194) (apache#5222) Signed-off-by: Wellington Chevreuil <wchevreuil@apache.org> (cherry picked from commit ece8d01) Change-Id: Ia5e78112b49c576e094c9479001e4de9abb006fe
The prefetch persistence feature requires setting evict on close to false to avoid evictions during a graceful restart. This however causes problems for normal moves (such as those triggered by balancer or manual movement), as now the blocks for moved regions would stay in the cache indefinitely.
Blocks in BucketCache should not be evicted when cache/prefetch persistence is enabled and should be evicted when a region is closed due to other reasons.
Jira: https://issues.apache.org/jira/browse/HBASE-27752