HBASE-27230 RegionServer should be aborted when WAL.sync throws Timeo… #4641
Conversation
This file contains 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. |
added 2 commits
July 22, 2022 14:09
|
🎊 +1 overall
This message was automatically generated. |
sunhelly
reviewed
Jul 22, 2022
|
|
||
| /** | ||
| * If {@link WAL#sync} get a timeout exception, the only correct way is to abort the region | ||
| * server, as the design of {@link WAL#sync}, is to succeed or die, there is no 'failure'. It |
There was a problem hiding this comment.
Actually we should avoid aborting the RS when the WAL sync is slow or timeout. As in the design of HBASE-22301 we should try to roll slow WALs to connect and rewrite to faster DNs. If you want to recover the flushing of WALs by aborting the RS, you will suffer from a whole MTTR but only the new WAL created by the newly started RS is helpful to the question.
There was a problem hiding this comment.
@sunhelly , thank you for review, yes , I could catch your point, here means after we does what you said, such as roll WALs and open a new writer and try to write the WAL entries again and again, and finally WAL.sync still timeout, we abort the RS. We set a very large default value here, 5 minutes, usually the WAL system will abort the region server if it can not finish the sync within 5 minutes, see HBASE-27233 and #4633 for more discussion.
There was a problem hiding this comment.
Got it, agree with the point 'no failure for WAL sync', whenever WAL.sync throws exception(no matter it is timeout or other), the regionserver should abort. This is a common logic.
|
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Jul 22, 2022
| * so there is no need to replicate to secondary replica, for this reason here we attach the | ||
| * region replication action after the {@link HRegion#sync} is successful. | ||
| */ | ||
| this.attachRegionReplicationInWALAppend(batchOp, miniBatchOp, walKey, walEdit, writeEntry); |
There was a problem hiding this comment.
Let's not include this change here? Should be a separated issue.
| /** | ||
| * Thrown when {@link WAL#sync} timeout. | ||
| */ | ||
| @SuppressWarnings("serial") |
There was a problem hiding this comment.
Just add a generated serial id?
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
added 3 commits
July 23, 2022 10:33
|
🎊 +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. |
Apache9
approved these changes
Jul 23, 2022
| @@ -62,9 +62,9 @@ void init(FileSystem fs, Path path, Configuration c, boolean overwritable, long | |||
| StreamSlowMonitor monitor) throws IOException, CommonFSUtils.StreamLacksCapabilityException; | |||
| } | |||
|
|
|||
| private EventLoopGroup eventLoopGroup; | |||
There was a problem hiding this comment.
Add comments to say this will be used in tests.
There was a problem hiding this comment.
@Apache9 ，ok ,fixed.
added 2 commits
July 23, 2022 22:12
|
💔 -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. |
jhungund
pushed a commit
to jhungund/hbase
that referenced
this pull request
Feb 26, 2024
apache#4641) Signed-off-by: Duo Zhang <zhangduo@apache.org> Change-Id: I4a413143681376235a250d164fec36ff9eb377b4
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
…throws Timeout apache#4641) Signed-off-by: Duo Zhang <zhangduo@apache.org> Change-Id: I4a413143681376235a250d164fec36ff9eb377b4
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…utIOException