Uh oh!
There was an error while loading. Please reload this page.
HBASE-27028 Add a shell command for flushing master local region - #4457
Conversation
Apache-HBase
commented
May 21, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 21, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 21, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 22, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 22, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 22, 2022
🎊 +1 overall
This message was automatically generated. |
Apache9
left a comment
There was a problem hiding this comment.
I prefer we add an extra rpc method and a shell command to do this, like flush_master_store, or something else.
We do not expose the name of master local region to upper layer, it is OK for a user to have a table with the same name.
Thanks for the review @Apache9. You are right, it may conflict with the user's 'master: store' table. |
5292647 to
21f9cc4CompareApache-HBase
commented
May 24, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 24, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 24, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 24, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
commented
May 24, 2022
The failures are caused by #4442 |
Apache-HBase
commented
May 24, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 24, 2022
🎊 +1 overall
This message was automatically generated. |
2005hithlj
commented
May 24, 2022
2005hithlj
commented
May 24, 2022
@carp84 Could you take a look? Thanks. |
Apache-HBase
commented
May 27, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 27, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 27, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
left a comment
There was a problem hiding this comment.
So the implementation is just to request a flush, not complete the flush, but the method name is called flush and we returns a CompletableFuture, which seems to tell users that joining on the future then you can wait for the flush to complete...
Better have a more clear design and naming of the method name and behavior.
Uh oh!
There was an error while loading. Please reload this page.
| return region.getScanner(scan); | ||
| } | ||
| public void rpcRequestFlush() { |
2005hithlj
commented
May 30, 2022
Thanks @Apache9 for the review. I will revise it according to your suggestion. |
Apache9
left a comment
There was a problem hiding this comment.
Why only requestFlush, not flush? We do not have a requestFlush method in the Admin interface in the past?
| /** | ||
| * Force request flush master local region | ||
| */ | ||
| void forceRequestFlushMasterStore() throws IOException; |
There was a problem hiding this comment.
I mean add a force flag... Not add a force to the method name...
Checked the code, we do not have this flag in the Admin interface, and at rs side, it will always use true when calling region.flush...
So sorry, let's keep align with other methods in this interface, just call it flushMasterStore, and at master side, we call region.flush(true).
There was a problem hiding this comment.
Thanks @Apache9 for the review.
If we call region.flush(true) at the master side, the changesAfterLastFlush value of MasterRegionFlusherAndCompactor will become incorrect, which may trigger an invalid flush operation.
But we can call MasterRegionFlusherAndCompactor.requestFlush() to avoid this.
Do we need to consider this situation?
There was a problem hiding this comment.
Then we could introduce a method in MasterRegionFlusherAndCompactor? Just reset changesAfterLastFlush after calling flush?
Anyway, what I mean is we should try our best to align the behavior in our Admin interface.
There was a problem hiding this comment.
Yes, we need to introduce a new method to reset the value of changesAfterLastFlush after calling flush.
Let me modify the code here. Thanks @Apache9 .
There was a problem hiding this comment.
I checked the code, because region.flush() is a synchronous method, we should call resetChangesAfterLastFlush() before calling region.flush() method, which should be similar to MasterRegionFlusherAndCompactor.flushLoop() method code logic. @Apache9
Apache-HBase
commented
May 31, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 31, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 31, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 1, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 1, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 2, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 2, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2022
🎊 +1 overall
This message was automatically generated. |
2005hithlj
commented
Jun 5, 2022
I have a new commit, could you take a look ? |
| private static final boolean DEFAULT_WARMUP_BEFORE_MOVE = true; | ||
| // Only for testing | ||
| private boolean isLocalRegionFlushed = false; |
There was a problem hiding this comment.
Is it possible to not adding a flag which is only used to verify test result? I think you can add a CP method for the flushMasterRegion call, and use attach a CP to verify that whether the master is called.
There was a problem hiding this comment.
Does CP here mean coprocessor? Is there a similar implementation example? sir @Apache9
| public FlushResult flush(boolean force) throws IOException { | ||
| return region.flush(force); | ||
| flusherAndCompactor.resetChangesAfterLastFlush(); |
There was a problem hiding this comment.
We do not need to record the lastFlushTime here?
Apache-HBase
commented
Jun 7, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 7, 2022
🎊 +1 overall
This message was automatically generated. |
2005hithlj
commented
Jun 8, 2022
@Apache9 I have a new commit, could you take a look? Thanks. |
Apache-HBase
commented
Jun 8, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
left a comment
There was a problem hiding this comment.
Overall LGTM. Just a naming problem. Will merge after you fix it.
Thanks @2005hithlj for the patient.
| changesAfterLastFlush.set(0); | ||
| } | ||
| void resetLastFlushTime() { |
There was a problem hiding this comment.
Should name it recordLastFlushTime?
There was a problem hiding this comment.
Thanks @Apache9 for the review.
I have modified it in the latest commit.
Apache-HBase
commented
Jun 10, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 11, 2022
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Jun 11, 2022
Checked the output, I think the problem is that, we run protoc test in the sub module directory so the session.executionRootDirectory is pointed to the sub module then we can not find license-header file. Let me file an issue about this. |
Apache-HBase
commented
Jun 11, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 11, 2022
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 12, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 12, 2022
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 12, 2022
💔 -1 overall
This message was automatically generated. |
2005hithlj
commented
Jun 13, 2022
In my local environment, the UT TestReplicationDroppedTables can be executed successfully. |
…che#4457) Signed-off-by: Duo Zhang <zhangduo@apache.org>
…ion (apache#4457)" This reverts commit 958ecc3.
https://issues.apache.org/jira/browse/HBASE-27028