HBASE-28456 HBase Restore restores old data if data for the same timestamp is in different hfiles #5775
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
bbeaudreault
force-pushed
the
HBASE-28456
branch
from
e788c26 to
cdef073
Compare
…stamp is in different hfiles
bbeaudreault
force-pushed
the
HBASE-28456
branch
from
cdef073 to
5c744b1
Compare
|
🎊 +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. |
ndimiduk
approved these changes
Mar 26, 2024
| @@ -165,7 +165,7 @@ protected static byte[] combineTableNameSuffix(byte[] tableName, byte[] suffix) | |||
| * package-private for internal usage for jobs like WALPlayer which need to use features of | |||
| * ExtendedCell. | |||
| */ | |||
| static final String EXTENDED_CELL_SERIALIZATION_ENABLED_KEY = | |||
| public static final String EXTENDED_CELL_SERIALIZATION_ENABLED_KEY = | |||
There was a problem hiding this comment.
This field is becoming part of the public API. It's worth updating the comment about its usage.
There was a problem hiding this comment.
@ndimiduk I decided to annotate that as IA.Private. I know we generally prefer not to do that for fields/methods, but for better or worse there's already a strong convention of doing it in this class (11 methods annotated IA.Private). It's probably worth a larger refactor/cleanup of HFileOutputFormat2
Let me know if you strongly disagree with this approach
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/HStoreFile.java
Outdated
Show resolved
Hide resolved
| @@ -423,6 +424,54 @@ public String toString() { | |||
| + (isReference() ? "->" + getReferredToFile(this.getPath()) + "-" + reference : ""); | |||
| } | |||
|
|
|||
| /** | |||
| * Cells in a bulkloaded file don't have a sequenceId since they don't go through memstore. When a | |||
| * bulkload file is committed, the current memstore ts is stamped onto the file name as the | |||
There was a problem hiding this comment.
Yikes! Seems like we need a seqId field in the file header instead of relying on the file name...
There was a problem hiding this comment.
I think the reason why we don't do this already is that a bulkloaded HFile is typically created outside the cluster. At that point, we don't know what the memstore seq id is at. Only at the time of bulkload file commit, when we've locked and flushed the memstore, do we know the memstore seq id.
Since hfiles are immutable, we can't open it at that point and change a metadata. So we need to add it to the filename I guess. I agree relying on the file name seems brittle. A more robust solution might be tricky given the above tho
|
🎊 +1 overall
This message was automatically generated. |
Apache9
approved these changes
Mar 26, 2024
|
|
||
| import static org.apache.hadoop.hbase.backup.BackupInfo.BackupState.COMPLETE; | ||
| import static org.apache.hadoop.hbase.backup.BackupType.FULL; | ||
| import static org.junit.Assert.*; |
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; | ||
| import java.time.Instant; | ||
| import java.util.*; |
| @@ -109,6 +114,9 @@ public boolean nextKeyValue() throws IOException, InterruptedException { | |||
| return false; | |||
| } | |||
| value = scanner.getCell(); | |||
| if (value != null && bulkloadSeqId.isPresent()) { | |||
| PrivateCellUtil.setSequenceId(value, bulkloadSeqId.getAsLong()); | |||
There was a problem hiding this comment.
We can not use bulkloadSeqId.ifPresent because PrivateCellUtil.setSequenceId will throw exceptions?
There was a problem hiding this comment.
Yea setSequenceId throws IOException. Figured it looks cleaner this way
bbeaudreault
added a commit
that referenced
this pull request
Mar 26, 2024
…stamp is in different hfiles (#5775) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
bbeaudreault
added a commit
that referenced
this pull request
Mar 26, 2024
…stamp is in different hfiles (#5775) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
bbeaudreault
added a commit
that referenced
this pull request
Mar 26, 2024
…stamp is in different hfiles (#5775) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
added a commit
to HubSpot/hbase
that referenced
this pull request
Mar 26, 2024
… for the same timestamp is in different hfiles (apache#5775) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Signed-off-by: Duo Zhang <zhangduo@apache.org>
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.
_SeqId_handling