-
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-27547 Close store file readers after region warmup #4942
Conversation
💔 -1 overall
This message was automatically generated. |
I think the problem here is we do not close the region after warmup? https://issues.apache.org/jira/browse/HBASE-13316 In the original commit, we will close the region after warmup... Let me see when we removed the r.close() line... Maybe it is just a mistake... |
OK, it is https://issues.apache.org/jira/browse/HBASE-15441. I think for warmup, the region should be opened as read only so it should not write any markers to WAL. And HBASE-15441 should have been included in 1.4.13, so this should not be the root cause of too many FDs? WDYT? @EungsopYoo Thanks. |
🎊 +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. |
Right. In 1.4.13, the warmed up region is not closed too. It's not the cause. |
@Apache9 We applied this patch to our cluster. And then the number of open FD decreased and never increased during RS rolling restart, which causes region move and warmup. It had been increased during RS rolling restart without this patch. |
I misunderstood your question. There are two points. The first is the leakage of open FDs during region moving(due to warmup), and the second is that HBase 2.4 opens much more FDs than HBase 1.4. In this PR, I fixed the first. The second is still in question. |
So I mean we should try to restore the old behavior, to close the warm up region? Now we support open a region in read only mode so when closing the region, we will not write any wal out, so it will not mess up wal splitting. Thanks. |
I think it's good to close warmed up region. Is it enough just to close the region? --- a/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HRegion.java
@@ -7773,6 +7773,7 @@ public class HRegion implements HeapSize, PropagatingConfigurationObserver, Regi
}
HRegion r = HRegion.newHRegion(tableDir, wal, fs, conf, info, htd, null);
r.initializeWarmup(reporter);
+ r.close();
}
/**
|
Ah
So set region to read only is not enough... Let me try if it is possible to set rsServices to null or wal to null here... |
Oh, we pass null as rsServices in HBASE-15441, so we will not mess up the sequence id, so closing the region will not introduce the problem described in HBASE-15441 back. In HBASE-15441, @elliottneilclark said
Setting the region to readOnly can prevent the flushing of edits, disabling all compactions is no harm. IIRC, there is a config to control whether we should evict block cache entries when closing a region. Let me check. |
EvictOnClose is a global config...
I think we could introduce a config here, like |
How about this? |
78cf4c9
to
cade1a1
Compare
cade1a1
to
e808205
Compare
💔 -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.
OK, good, saving the warmup flag is also a good way to fix it, as if we set warmup to true,it does not make sense to still clear the cache entries.
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 45fd3f6)
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 45fd3f6)
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 45fd3f6)
patch to branch-2.5 is failing the build , it would need a import line for TableDescriptor in hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestWarmupRegion.java. I may add an addendum for it in branch-2.5 and branch-2 (branch-2.4 has it) , or open PR if I'm too late for appending an addendum
|
Just do it, when applying HBASE-27598 to branch-2 I also found the problem and include the fix when cherry-picking HBASE-27598, so you just need to fix branch-2.5. Sorry for that. |
Signed-off-by: Duo Zhang <[email protected]> (cherry picked from commit 45fd3f6) (cherry picked from commit 79ecc22) Change-Id: I6c77d5f1179f0d568be46c2a55343ed86522f48a
No description provided.