-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-26867 Introduce a FlushProcedure #4246
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
Conversation
💔 -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. |
💔 -1 overall
This message was automatically generated. |
// the hbase hadoop version does not match the running hadoop version. | ||
// if that happens, we need fall back to the old flush implementation. | ||
LOG.info("Unrecoverable error in master side. Fallback to FlushTableProcedure V1", error); | ||
addListener(tableExists(tableName), (exists, err) -> { |
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.
Better abstract this to a seprated method and call it legacyFlush(or something else, I'm not good at naming in English...)
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. Thanks Duo.
* the procedure will suspend and retry later. | ||
*/ | ||
@InterfaceAudience.Private | ||
public abstract class AbstractRegionRemoteProcedure extends Procedure<MasterProcedureEnv> |
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.
Better give it another name? Like IdempotentRegionRemoteProcedureBase?
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. Thanks Duo.
false, FlushLifeCycleTracker.DUMMY); | ||
} | ||
if (res.getResult() == HRegion.FlushResult.Result.CANNOT_FLUSH) { | ||
region.waitForFlushes(); |
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.
Is it always safe to call this here?
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.
From what I've seen so far, it's safe. It's just for a simple check to make sure all the data which should be flushed (whose seqId is less than or equal to the read point) has been flushed. But it's ok for me to remove this check if you're concerned there is any risk of getting the flush procedure stuck here.
Thanks so much for reviewing this Duo. I'll address the suggestions as soon as possible. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Any updates here? |
Sorry, I've been a little busy recently. Will start work on fixing file conflicts as soon as possible. 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. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
hi @frostruan , any updates here ? |
Thanks for your attention guangxu @guangxuCheng . Will continue this work as soon as possible. |
Submitted a new PR #5256 . This one will be closed. Please see the new PR if you're interested, I'd be grateful for any comments and suggestions. Thanks. @Apache9 @guangxuCheng |
No description provided.