HBASE-22539 WAL corruption due to early DBBs re-use when Durability.A… #437
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. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
openinx
reviewed
Aug 2, 2019
| @@ -1061,8 +1063,10 @@ protected final long stampSequenceIdAndPublishToRingBuffer(RegionInfo hri, WALKe | |||
| txidHolder.setValue(ringBuffer.next()); | |||
| }); | |||
| long txid = txidHolder.longValue(); | |||
| ServerCall<?> rpcCall = RpcServer.getCurrentCall().filter(c -> c instanceof ServerCall) | |||
There was a problem hiding this comment.
The currentCall should always be a ServerCall I think, so no need the extra instanceof ? It's also OK, if you think it's fine.
| @@ -1002,6 +1001,8 @@ public void onEvent(final RingBufferTruck truck, final long sequence, boolean en | |||
| : new DamagedWALException("On sync", this.exception)); | |||
| // Return to keep processing events coming off the ringbuffer | |||
| return; | |||
| } finally { | |||
| entry.getRpcCall().ifPresent(ServerCall::releaseByWAL); | |||
There was a problem hiding this comment.
Good. Releasing the BB here should be fine, because once the BB was written into the outputStream, it would be copied to a new memory area, won't depend on the rpc BB now
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AsyncFSWAL.java
Outdated
Show resolved
Hide resolved
| @@ -487,6 +490,7 @@ private void drainNonMarkerEditsAndFailSyncs() { | |||
| while (iter.hasNext()) { | |||
| FSWALEntry entry = iter.next(); | |||
| if (!entry.getEdit().isMetaEdit()) { | |||
| entry.getRpcCall().ifPresent(ServerCall::releaseByWAL); | |||
There was a problem hiding this comment.
In theory, once we remove a FSWALEntry from the unackedAppended deque, we should release the BB backend the WAL ? Could we define a deque subclass, once we remove the entry from it then will release it , then we don't need to release the wal everywhere, I think...
There was a problem hiding this comment.
No, if there are errors when syncing, we will put all the entries in unackedAppends back to toWriteAppends, and there we should not release the entries.
openinx
reviewed
Aug 2, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSWALEntry.java
Show resolved
Hide resolved
openinx
reviewed
Aug 2, 2019
|
🎊 +1 overall
This message was automatically generated. |
wchevreuil
reviewed
Aug 2, 2019
| @@ -141,14 +147,43 @@ public void done() { | |||
| cleanup(); | |||
| } | |||
|
|
|||
| private void release(int mask) { | |||
| for (;;) { | |||
There was a problem hiding this comment.
Should rather wait for notifications from cleanup()?
There was a problem hiding this comment.
Sorry, I can not get your point. What's the problem here?
There was a problem hiding this comment.
I meant to put a wait call inside this for block, so that it does not keep iterating indefinitely. But I guess condition on #153 will already halt it on the 2nd iteration, so should be fine.
There was a problem hiding this comment.
This is for CAS. You can see the code in AtomicInteger.getAndUpdate, almost the same.
wchevreuil
reviewed
Aug 2, 2019
I think the message means this should not happen, so we do not need to deal with it? |
|
💔 -1 overall
This message was automatically generated. |
|
The failed UT is unrelated. @openinx @wchevreuil Any other concerns? This is a critical bug, we should roll new releases soon. |
openinx
reviewed
Aug 4, 2019
There was a problem hiding this comment.
I still think we should release the BB in the exceptional case for AsyncFSWAL#appendAndSync,
try {
appended = append(writer, entry);
} catch (IOException e) {
throw new AssertionError("should not happen", e);
}
There're coprocessorHost.preWALWrite inside the append method, although the Assert message say it shouldn't happen, but user may write an incorrect CP, which would throw an IOException..
Also, I think better to change the message "Shouldn't happen..."
openinx
reviewed
Aug 4, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/AsyncFSWAL.java
Outdated
Show resolved
Hide resolved
If this happens we just let the region server die, I think this is the correct way to deal with this. The design here can not deal with the IOException. |
openinx
reviewed
Aug 4, 2019
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSWALEntry.java
Show resolved
Hide resolved
wchevreuil
reviewed
Aug 4, 2019
| } | ||
| } | ||
| } | ||
|
|
||
| @Override |
There was a problem hiding this comment.
So here we rely that cleanup() will never be called before retainByWal(), otherwise the rpc call may end before the wal write is done?
There was a problem hiding this comment.
Yes, we should call retainByWal before cleanup. So we must call retainByWal in the rpc handler thread, before putting it into the ringbuffer of WAL, I think this is enough to make sure that we call retainByWal before cleanup.
|
🎊 +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. |
|
Let me merge this back if no other big concerns. This is a critical bug... |
|
💔 -1 overall
This message was automatically generated. |
asfgit
pushed a commit
that referenced
this pull request
Aug 5, 2019
…SYNC_WAL is used (#437) Signed-off-by: Zheng Hu <openinx@gmail.com>
asfgit
pushed a commit
that referenced
this pull request
Aug 5, 2019
…SYNC_WAL is used (#437) Signed-off-by: Zheng Hu <openinx@gmail.com>
asfgit
pushed a commit
that referenced
this pull request
Aug 5, 2019
…SYNC_WAL is used (#437) Signed-off-by: Zheng Hu <openinx@gmail.com>
asfgit
pushed a commit
that referenced
this pull request
Aug 5, 2019
…SYNC_WAL is used (#437) Signed-off-by: Zheng Hu <openinx@gmail.com>
|
💔 -1 overall
This message was automatically generated. |
infraio
pushed a commit
to infraio/hbase
that referenced
this pull request
Aug 17, 2020
…SYNC_WAL is used (apache#437) Signed-off-by: Zheng Hu <openinx@gmail.com>
symat
pushed a commit
to symat/hbase
that referenced
this pull request
Feb 17, 2021
…SYNC_WAL is used (apache#437) Signed-off-by: Zheng Hu <openinx@gmail.com> (cherry picked from commit 2806d1d) Change-Id: I069ebca03aa493f5266d1c1605459359dd6e8457
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.
…SYNC_WAL is used