HBASE-27483 Expose table and region storefiles accessed days and size to the metrics #4891
+478
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
taklwu
reviewed
Nov 21, 2022
| public static long ONE_DAY_MS = 24 * 3600 * 1000; | ||
| public static final String STOREFILES_ACCESSED_DAYS_THRESHOLDS = | ||
| "hbase.region.storefiles.accessed.days.thresholds"; | ||
| public static final int[] STOREFILES_ACCESSED_DAYS_THRESHOLDS_DEFAULT = { 7, 30, 90 }; |
There was a problem hiding this comment.
can we add few line of javadoc or some line in README.md how to use this ?
There was a problem hiding this comment.
One more question, if we are using cache / bucket cache heavily, will this metric retain at the time the store file flushes or opened for cache? It’s fine as is without caring the cache , just want to learn more here
There was a problem hiding this comment.
Yeah, in this situation, the metrics will retain at the time the store file flushes or opened for cache, although the cached data of store file is accessed at a new time. I'm not sure whether it is critical issue.
| @Override | ||
| public void run() { | ||
| Map<TableName, MetricsTableValues> localMetricsTableMap = new HashMap<>(); | ||
|
|
There was a problem hiding this comment.
[nit] is this added by spotless?
Suggested change
Comment on lines
440
to
442
| metricsHelper.assertGaugeGt(prefix + "storeFilesAccessed7DaysSize", 0, agg); | ||
| metricsHelper.assertGaugeGt(prefix + "storeFilesAccessed30DaysSize", 0, agg); | ||
| metricsHelper.assertGaugeGt(prefix + "storeFilesAccessed90DaysSize", 0, agg); |
There was a problem hiding this comment.
[nit] is there any way to verify non-zero metric for storeFilesAccessed%sDaysSize in test, e.g. if we can mock the access time ?
|
@taklwu sir. Thanks for your review. Please wait for my new commit. |
|
@taklwu sir, I'm sorry to have kept you waiting so long. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
@taklwu Could you take a look? Thanks. |
Apache9
reviewed
Dec 22, 2022
| String ROW_READS_ONLY_ON_MEMSTORE_DESC = "Row reads happening completely out of memstore"; | ||
| String MIXED_ROW_READS = "mixedRowReadsCount"; | ||
| String MIXED_ROW_READS_ON_STORE_DESC = "Row reads happening out of files and memstore on store"; | ||
| String STOREFILES_ACCESSED_DAYS_AND_SIZE = "storeFilesAccessed%sDaysSize"; |
There was a problem hiding this comment.
Better name it XXX_TEMPLATE.
|
|
||
| Map<String, Pair<Long, Long>> sfAccessTimeAndSizeMap = | ||
| store.getStoreFilesAccessTimeAndSize(); | ||
| Long now = EnvironmentEdgeManager.currentTime(); |
There was a problem hiding this comment.
Use long instead of Long as later we will do calculation?
There was a problem hiding this comment.
Use
longinstead ofLongas later we will do calculation?
OK, sir.
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStore.java
Show resolved
Hide resolved
2005hithlj
force-pushed
the
HBASE-27483
branch
from
a59d412 to
c21b0ab
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
The failed UTs seems to have nothing to do with my modification. |
|
@Apache9 sir. Could you take a look? Thanks. |
|
Please fix the conflicts. And how do you plan to reduce the pressure on NN when fetching the accessTime? Or is this not a big problem? Thanks. |
|
🎊 +1 overall
This message was automatically generated. |
@Apache9 sir. I have a new commit. Could you take a look? Thanks. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
May 15, 2023
| mt.perStoreMixedReadCount.put(tempKey, mixedReadCount); | ||
| Map<String, Pair<Long, Long>> sfAccessTimeAndSizeMap = | ||
| store.getStoreFilesAccessTimeAndSize(); | ||
| Long now = EnvironmentEdgeManager.currentTime(); |
There was a problem hiding this comment.
Just use long instead of Long?
| Long size = pair.getSecond(); | ||
| for (int threshold : storeFilesAccessedDaysThresholds) { | ||
| Long sumSize = mt.storeFilesAccessedDaysAndSize.get(threshold); | ||
| if ((now - accessTime) >= threshold * ONE_DAY_MS) { |
There was a problem hiding this comment.
So here if the accessTime is very old, we will increase the sumSize for all the thresholds? Is this by design? Seems a bit strange...
There was a problem hiding this comment.
@Apache9 sir.
Yes, this is by design. For instance, if an HFile has not been accessed for 90 days, it is certain that it has also not been accessed for 30 and 7 days.
And what is your suggestion?
There was a problem hiding this comment.
OK, so the metric here is 'the number if store files which are not accessed by at least xx days'? Then I think we;d better have a better name for it? Looking at 'storeFilesAccessedDays', I can not get the meaning unless you explain it...
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Any updates here? @2005hithlj Thanks. |
@Apache9 Sorry sir, I've been a bit busy at work lately. I don't have extra bandwidth to do anything else at the moment. |
|
OK, then we can move it out of 3.0.0 first. Can come back later when you have time. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.apache.org/jira/browse/HBASE-27483