Uh oh!
There was an error while loading. Please reload this page.
HBASE-26867 Introduce a FlushProcedure - #5256
Merged
Merged
Conversation
Apache-HBase
commented
May 27, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
May 27, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
May 27, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jun 2, 2023
Apache9
left a comment
Contributor
There was a problem hiding this comment.
Overall LGTM.
The only concern is about how we implement the procedure lock.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| FlushLifeCycleTracker.DUMMY); | ||
| } | ||
| if (res.getResult() == HRegion.FlushResult.Result.CANNOT_FLUSH) { | ||
| region.waitForFlushes(); |
Contributor
There was a problem hiding this comment.
Should we wait here? Or just fail the remote procedure call, and let the master schedule again?
ContributorAuthor
There was a problem hiding this comment.
I think you are right, the current implementation may stuck the flush executor, which is not good. It might be a better idea to fail fast and try again. Let me fix this. Thanks Duo.
| if (columnFamily == null) { | ||
| res = region.flush(true); | ||
| } else { | ||
| res = region.flushcache(Collections.singletonList(columnFamily), false, |
Contributor
There was a problem hiding this comment.
Do we want to support flush several families at once? Do we have this support in the old flush procedure?
ContributorAuthor
There was a problem hiding this comment.
Yes. The flush procedure v1 supports flushing the specified column family on all regions. So I want to make flush procedure v2 have the same ability. :)
Contributor
There was a problem hiding this comment.
I mean do we need to add support to pass multiple column families here? Not only one.
Apache-HBase
commented
Jun 3, 2023
💔 -1 overall
This message was automatically generated. |
2 similar comments
Apache-HBase
commented
Jun 3, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 3, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 3, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 3, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 3, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 4, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jun 16, 2023
| throw new UnsupportedOperationException("unhandled state=" + state); | ||
| } | ||
| } catch (Exception e) { | ||
| setFailure("master-flush-table", e); |
Contributor
There was a problem hiding this comment.
What could casue the procedure failure? Since we do not support rollback, I do not think we can mark this procedure as failure after entering the fllush regions state...
ContributorAuthor
There was a problem hiding this comment.
Thank you for pointing this out Duo, it is necessary that we adjust it after we make the FlushTableProcedure not support rollback.
My original thought was this, when we are in the state FLUSH_TABLE_PREPARE, we need to confirm that the state of the table is Online, which may require an RPC to access the meta table, because the TableStateManager loads the state of the table lazily. if the request fails, an exception may be thrown, so I originally thought that if the request fails, then just roll back the procedure.
Since we do not support rollback now, let me fix this.
Apache-HBase
commented
Jun 26, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 26, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jun 26, 2023
💔 -1 overall
This message was automatically generated. |
Apache9
reviewed
Jun 28, 2023
Uh oh!
There was an error while loading. Please reload this page.
| if (columnFamily == null) { | ||
| res = region.flush(true); | ||
| } else { | ||
| res = region.flushcache(Collections.singletonList(columnFamily), false, |
Contributor
There was a problem hiding this comment.
I mean do we need to add support to pass multiple column families here? Not only one.
frostruan
commented
Jun 28, 2023
ContributorAuthor
OK. Let me improve this to support flushing multiple column families. Thanks Duo. |
Apache9
commented
Jul 5, 2023
Contributor
Any updates here? I think we are very close to get this in. Thanks. |
Apache-HBase
commented
Jul 7, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 7, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 31, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 31, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 31, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 5, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 5, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 5, 2023
💔 -1 overall
This message was automatically generated. |
Apache9
approved these changes
Aug 13, 2023
Apache9
commented
Aug 13, 2023
Contributor
Triggered another build to see if we can get an all green. Overall LGTM. Thanks @frostruan for your patience. |
Apache-HBase
commented
Aug 13, 2023
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 13, 2023
🎊 +1 overall
This message was automatically generated. |
frostruan
commented
Aug 14, 2023
ContributorAuthor
Thanks Duo for still following this ! @Apache9 The spotless problem looks irrelevant. Should we trigger another build ? |
Apache9
commented
Aug 14, 2023
Contributor
It is not related. But please fix the javac warning? We missed an Override annotation. Thanks. |
frostruan
commented
Aug 14, 2023
ContributorAuthor
A new commit has been added. Let's wait for the UT results. REALLY appreciate your help Duo. @Apache9 |
Apache-HBase
commented
Aug 14, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 14, 2023
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 14, 2023
🎊 +1 overall
This message was automatically generated. |
Apache9 pushed a commit
that referenced
this pull request
Aug 14, 2023
Co-authored-by: huiruan <876107431@qq.com> Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 20c9e4b)
| ForeignExceptionDispatcher monitor = new ForeignExceptionDispatcher(desc.getInstance()); | ||
| HBaseProtos.NameStringPair family = null; |
Member
There was a problem hiding this comment.
It looks like this segment was missed in the backport to branch-2. I noticed because I'm backporting HBASE-28187 / #5692.
@frostruan@Apache9 is there a semantic difference between the value of family/families between master/branch-3 and branch-2? Or can I apply this rename without concern? Thanks.
Member
There was a problem hiding this comment.
I have not done an audit of the PRs for master vs. branch-2, there may be other missing patch segments.
ContributorAuthor
There was a problem hiding this comment.
From the perspective of program correctness, there is no difference between family and families. From a semantic point of view, this string supports multiple column family names separated by commas, such as "cf1,cf2, cf3", so I think families is more appropriate here. In fact, in RegionServerFlushTableProcedureManager, this parameter is also named families.So I think you can rename it safely. Thanks.
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
Sign up for freeto 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.
No description provided.