-
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-27732 NPE in TestBasicWALEntryStreamFSHLog.testEOFExceptionInOl… #5119
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. |
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.
Looks like mvninstall
failure above is flaky one right?
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.
Changes look good overall
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AbstractFSWAL.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AsyncFSWAL.java
Show resolved
Hide resolved
I am +1 for the changes, but shall we update PR/Jira title to reflect changes done beyond test fix? Something like |
We could add more detailed information in the commit message as well as on the jira issue's description. Let me update. |
@virajjasani PTAL again. Thanks. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
Let me check the failed UTs. |
💔 -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. |
Oh, there is a data race if we also call cleanOldLogs in closeExecutor... Let me think how to better fix this, maybe a simple synchronized is enough as cleanOldLogs are all in memory operations after we introduced a special thread pool for archiving. |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
…dWALsDirectory Add a 'closed' flag in WALProps in AbstractFSWAL to indicate that whether a WAL file has been closed, if not, we will not try to archive it. Will mark it as closed after we fully close it in the background close task, and try to archive again. Also modified some tests since now the archiving of a rolled WAL file is also asynchronous, we need to wait instead of asserting directly.
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Okk, was this the only change |
💔 -1 overall
This message was automatically generated. |
Yes, I added more comments for this method to explain why we need synchronized here. The failed UT is TestFuzzyRowFilterEndToEnd, which is not related. Let me trigger the UT again to confirm that there are no new flaky UTs. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
+1, thanks @Apache9 |
…dWALsDirectory (#5119) Add a 'closed' flag in WALProps in AbstractFSWAL to indicate that whether a WAL file has been closed, if not, we will not try to archive it. Will mark it as closed after we fully close it in the background close task, and try to archive again. Also modified some tests since now the archiving of a rolled WAL file is also asynchronous, we need to wait instead of asserting directly. Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 230fdc0)
…dWALsDirectory (#5119) Add a 'closed' flag in WALProps in AbstractFSWAL to indicate that whether a WAL file has been closed, if not, we will not try to archive it. Will mark it as closed after we fully close it in the background close task, and try to archive again. Also modified some tests since now the archiving of a rolled WAL file is also asynchronous, we need to wait instead of asserting directly. Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 230fdc0)
…dWALsDirectory (#5119) Add a 'closed' flag in WALProps in AbstractFSWAL to indicate that whether a WAL file has been closed, if not, we will not try to archive it. Will mark it as closed after we fully close it in the background close task, and try to archive again. Also modified some tests since now the archiving of a rolled WAL file is also asynchronous, we need to wait instead of asserting directly. Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 230fdc0)
…dWALsDirectory (apache#5119) Add a 'closed' flag in WALProps in AbstractFSWAL to indicate that whether a WAL file has been closed, if not, we will not try to archive it. Will mark it as closed after we fully close it in the background close task, and try to archive again. Also modified some tests since now the archiving of a rolled WAL file is also asynchronous, we need to wait instead of asserting directly. Signed-off-by: Viraj Jasani <vjasani@apache.org>
…dWALsDirectory (apache#5119) Add a 'closed' flag in WALProps in AbstractFSWAL to indicate that whether a WAL file has been closed, if not, we will not try to archive it. Will mark it as closed after we fully close it in the background close task, and try to archive again. Also modified some tests since now the archiving of a rolled WAL file is also asynchronous, we need to wait instead of asserting directly. Signed-off-by: Viraj Jasani <vjasani@apache.org> (cherry picked from commit 230fdc0) (cherry picked from commit e95b47e) Change-Id: I29133cfe4b4695bf817b7abb1cca775f4a5eb88a
…dWALsDirectory