Uh oh!
There was an error while loading. Please reload this page.
HBASE-23157 WAL unflushed seqId tracking may wrong when Durability.AS… - #762
Merged
Conversation
Apache9
commented
Oct 27, 2019
ContributorAuthor
@saintstack@binlijin PTAL. Thanks. |
Apache-HBase
commented
Oct 27, 2019
💔 -1 overall
This message was automatically generated. |
saintstack
left a comment
Contributor
There was a problem hiding this comment.
I like the idea. Worried about incrementing sequenceids outside of mvcc.
| // then it will be recorded as the lowestUnflushedSeqId by the above update method, which is | ||
| // less than the current maxFlushedSeqId. And if next time we only flush the family with this | ||
| // unusual lowestUnflushedSeqId, the maxFlushedSeqId will go backwards. | ||
| // This is an unexpected behavior so we should fix it, otherwise it may cause unexpected |
Contributor
There was a problem hiding this comment.
How we going to fix it? Can't we drive through the flush marker before completing the flush?
ContributorAuthor
There was a problem hiding this comment.
Then we need to do a wal sync under the updateLock.writeLock, which will impact the performance.
Contributor
There was a problem hiding this comment.
... maybe a slight hiccup for the async wal case -- maybe, given sync'ing to HDFS is such an erratic affair -- but we will also be more 'correct' having pushed out all in the ring buffer ahead of this flush sync w/o having to rewrite their sequenceid.
| // This is an unexpected behavior so we should fix it, otherwise it may cause unexpected | ||
| // behavior in other area and then cause data loss maybe. | ||
| // The solution here is a bit hack but fine. Just replace the lowestUnflushedSeqId with | ||
| // maxFlushedSeqId + 1 if it is lesser. Durability.ASYNC_WAL means we do not care data loss for |
Contributor
There was a problem hiding this comment.
Should we +1 it and not just let it be the maxFlushedSeqID? The maxFlushedSeqId comes from mvcc. +1'ing outside of mvcc management could have an interesting side-effect not seen currently where next edit given out by mvcc might clash w/ this +1'd record.
ContributorAuthor
There was a problem hiding this comment.
See the comment below, I explain why +1 is safe. The way we get the maxFlushedSeqId is mvcc -1, so here we just add it back.
Contributor
There was a problem hiding this comment.
Yeah. I read that. Seems fragile. Logic is spread about being here and up in caller. Messing w/ sequenceid w/o going via mvcc will bite us later I think.
We have to do this math?
ContributorAuthor
There was a problem hiding this comment.
It is all about the meaning of these sequence ids. The maxFlushedSeqId means the maximum flush id that have been flushed, so it must be the current lowest sequence id in memstore - 1, otherwise there will be data loss for the edge record. And here we store the current lowest sequence id in memstore so we have to +1 to get the maxFlushedSeqId.
Contributor
There was a problem hiding this comment.
Are the edits on the ring buffer that are outstanding meant to be part of this flush or are they supposed to be counted in the next flush? Are you adding the +1 so they are not part of the current flush?
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Apache-HBase
commented
Oct 28, 2019
🎊 +1 overall
This message was automatically generated. |
binlijin
commented
Oct 28, 2019
Contributor
LGTM |
Apache-HBase
commented
Dec 4, 2019
💔 -1 overall
This message was automatically generated. |
Apache9
commented
Dec 5, 2019
ContributorAuthor
Oh, this one is still pending? Any other concerns? @saintstack |
Apache-HBase
commented
Dec 7, 2019
💔 -1 overall
This message was automatically generated. |
saintstack
left a comment
Contributor
There was a problem hiding this comment.
Took another look after doing a bit of study. I could go for this 'hack' if we removed the +1. I could work on a follow-on that does the suggested drive through of the sync edit if that'd help.
Uh oh!
There was an error while loading. Please reload this page.
| // then it will be recorded as the lowestUnflushedSeqId by the above update method, which is | ||
| // less than the current maxFlushedSeqId. And if next time we only flush the family with this | ||
| // unusual lowestUnflushedSeqId, the maxFlushedSeqId will go backwards. | ||
| // This is an unexpected behavior so we should fix it, otherwise it may cause unexpected |
Contributor
There was a problem hiding this comment.
... maybe a slight hiccup for the async wal case -- maybe, given sync'ing to HDFS is such an erratic affair -- but we will also be more 'correct' having pushed out all in the ring buffer ahead of this flush sync w/o having to rewrite their sequenceid.
| // means we have flushed all the stores so the seq id for actual data should be at least plus 1. | ||
| // And if we do not flush all the stores, then the maxFlushedSeqId is calculated by | ||
| // lowestUnflushedSeqId - 1, so here let's plus the 1 back. | ||
| Long wrappedSeqId = Long.valueOf(maxFlushedSeqId + 1); |
Contributor
There was a problem hiding this comment.
The +1 makes me wary. Just add these edits w/ maxFlushedSeqId? Wouldn't that be safer.
ContributorAuthor
There was a problem hiding this comment.
I explained why we need to +1 here, so what's your real problem? If you do not want to +1, you need to change bunch of comments and field names...
Contributor
There was a problem hiding this comment.
Makes sense.
Your comment/explanation here is good. It is also where it should be, in the class named SequenceIdAccounting. Though hard to follow, it should be possible to read this one class to find how sequnceid accounting is done in the system including tricks to keep the system working when the likes of ASYNC_WAL is enabled.
Uh oh!
There was an error while loading. Please reload this page.
Apache9
commented
Jul 27, 2020
ContributorAuthor
@saintstack Let's finish this one? |
Apache-HBase
commented
Jul 27, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 27, 2020
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 27, 2020
🎊 +1 overall
This message was automatically generated. |
saintstack
approved these changes
Aug 9, 2020
| // means we have flushed all the stores so the seq id for actual data should be at least plus 1. | ||
| // And if we do not flush all the stores, then the maxFlushedSeqId is calculated by | ||
| // lowestUnflushedSeqId - 1, so here let's plus the 1 back. | ||
| Long wrappedSeqId = Long.valueOf(maxFlushedSeqId + 1); |
Contributor
There was a problem hiding this comment.
Makes sense.
Your comment/explanation here is good. It is also where it should be, in the class named SequenceIdAccounting. Though hard to follow, it should be possible to read this one class to find how sequnceid accounting is done in the system including tricks to keep the system working when the likes of ASYNC_WAL is enabled.
asfgit pushed a commit
that referenced
this pull request
Aug 10, 2020
…YNC_WAL is used (#762) Signed-off-by: stack <stack@apache.org>
asfgit pushed a commit
that referenced
this pull request
Aug 10, 2020
…YNC_WAL is used (#762) Signed-off-by: stack <stack@apache.org>
asfgit pushed a commit
that referenced
this pull request
Aug 10, 2020
…YNC_WAL is used (#762) Signed-off-by: stack <stack@apache.org>
clarax pushed a commit
to clarax/hbase
that referenced
this pull request
Nov 15, 2020
…YNC_WAL is used (apache#762) Signed-off-by: stack <stack@apache.org>
wchevreuil pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
…YNC_WAL is used (apache#762) Signed-off-by: stack <stack@apache.org> (cherry picked from commit 1cbe383) Change-Id: Ibdf7ab100974408b97d745d35fb55e59f2a6b82c
This file contains hidden or 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
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
…YNC_WAL is used