-
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-28637 asyncwal should attempt to recover lease if close fails #5962
Conversation
Change-Id: I08dc88c8d6ba4e23754ef2acd1e14b32b225ae17
💔 -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. |
FSHLog does not need this logic? |
try { | ||
RecoverLeaseFSUtils.recoverFileLease(fs, p, conf, null); | ||
} catch (IOException ex) { | ||
LOG.warn("Unable to recover lease after several attempts. Give up.", ex); |
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.
It should be an error.
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.
done.
Change-Id: Id2906597d4709d66db370886598f339cd77ee754
@Apache9 updated thanks for the review. |
💔 -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. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
RecoverLeaseFSUtils.recoverFileLease(fs, p, conf, null); | ||
} catch (IOException ex) { | ||
LOG.error("Unable to recover lease after several attempts. Give up.", ex); | ||
throw new RuntimeException(ex); |
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 not do this...
We are running in a background thread pool and this will kill the thread in the thread pool...
If the thread pool can still create new threads then we are safe, but it is useless to throw the exception out here. And if the thread pool is broken, then we are in trouble...
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.
Good point. How about just log the exception and move on?
Change-Id: I68d3b6e15c141433321c58b66b8de9efb10b171d
🎊 +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. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Thanks @Apache9 ! |
…pache#5962) Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 0e8cfdb) (cherry picked from commit b881296) Conflicts: hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java Change-Id: Ia704c1b36b99be345d11a70ac2452dea094e9575
…lose fails (apache#5962) (apache#6033) Signed-off-by: Duo Zhang <zhangduo@apache.org> (cherry picked from commit 0e8cfdb) (cherry picked from commit b881296) Change-Id: I6ad1d3122a0452d583a7269be9dbc3851f4f3a37
Do lease recovery as a last resort if async wal fails to close WAL file.
This issue will make WAL on Ozone more resilient because Ozone does not support renaming open files (which happens due to WAL log archiving).