HBASE-26791 Memstore flush fencing issue for SFT #4202
Merged
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. |
joshelser
reviewed
Mar 12, 2022
There was a problem hiding this comment.
Maybe I'm missing it, but I think there's still a chance for race condition here. Let me try to summarize how I think you're solving this and you can correct me.
We add a sequence id (the current timestamp or one more than the previous ID if we have a clock skew) for each store's tracker. This sequence id is included in the filename for uniqueness. When we write a new tracker out, we increment the id. When we go to load a store, we find the largest ID and pick that seqID's tracker file to load. After we pick the biggest seqId tracker, we delete any trackers older than the one we picked.
- RS1 makes a tracker for storeA with seq=1
- RS1 goes zombie
- RS2 gets assignment for storeA opens seq=2
- RS1 wakes up and tries to memstore flush
I'm confused because I feel like we will call load(false) which would just write a new tracker file with the newer time (or seqId + 1). I will totally admit I am probably just getting it by reading the code.
| // since read only is true, we should not delete the old track file | ||
| // the deletion is in background, so sleep a bit then assert | ||
| Thread.sleep(2000); | ||
| assertTrue(fs.exists(trackFileStatus.getPath())); |
There was a problem hiding this comment.
Use the HTU.Waiter to check a few times please.
Ah, so is it that we use the sequenceId only when opening the Region (store). After the region is open, choosing whether to use f1 or f2 is then based on what Sorry I missed your explanation on Jira. |
|
Some general thinking about this (not specific to your changes)
|
Ah you have missed a very important point... We will only bump the sequence id when loading at the primary replica(i.e, readOnly = false), this means we will only bump the sequence id when opening a region, so the scenario you described above has no problem. RS1 will only write the files with old sequence id while RS2 will write the files with new(greater) sequence id. This is the key trick here to solve the problem. |
As explained above, the key here is to make sure that the sequence id of the SFT files written by the new RS is greater than the ones written by the old RS. So the answer of these two questions are pretty easy:
|
wchevreuil
reviewed
Mar 17, 2022
There was a problem hiding this comment.
Overall, LGTM. Had made some nit comments on the subject of logging/supportability.
Test wise, could we add a test that mimics the issue, by instantiate two StoreFileListFile instances, update both with different store files, then check these updates don't reflect on each other lists of store files?
...r/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileListFile.java
Show resolved
Hide resolved
...r/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileListFile.java
Show resolved
Hide resolved
...r/src/main/java/org/apache/hadoop/hbase/regionserver/storefiletracker/StoreFileListFile.java
Show resolved
Hide resolved
|
🎊 +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. |
|
Any other concerns? @wchevreuil @joshelser |
Oh that's tricky. I didn't even think about that case. 👍🏼 |
wchevreuil
approved these changes
Mar 21, 2022
Apache9
added a commit
that referenced
this pull request
Mar 22, 2022
Signed-off-by: Josh Elser <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> (cherry picked from commit e56ed40)
vinayakphegde
pushed a commit
to vinayakphegde/hbase
that referenced
this pull request
Apr 4, 2024
Signed-off-by: Josh Elser <[email protected]> Signed-off-by: Wellington Chevreuil <[email protected]> (cherry picked from commit e56ed40) Change-Id: If8773e1fd5147fb339a084bfd6b6a34dbd6f4950
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.
No description provided.