HBASE-27649 WALPlayer does not properly dedupe overridden cell versions #5047
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
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
bbeaudreault
force-pushed
the
HBASE-27649
branch
from
8426f4d to
45091db
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. |
Apache9
approved these changes
Feb 25, 2023
| final byte[] row = Bytes.toBytes("row"); | ||
| Table table = TEST_UTIL.createTable(tableName, family); | ||
|
|
||
| long now = System.currentTimeMillis(); |
There was a problem hiding this comment.
Use EnvironmentEdgeManager.currentTime if possible
| table = TEST_UTIL.truncateTable(tableName); | ||
| g = new Get(row); | ||
| result = table.get(g); | ||
| assertTrue("expected row to be empty after truncate but got " + result, result.isEmpty()); |
There was a problem hiding this comment.
Use assertThat and Matchers.empty?
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
checkstyle/spotbugs are in hbase-backups module, which i haven't changed here. Will be handling that in a separate issue. Otherwise pre-commit passing so going to merge. Thanks! |
|
Ugh, somehow some temp code got in here in hbase-backup. Going to revert |
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.
This seems simple enough from a compatibility perspective. I'm adding a new
io.serializations, but it should not matter for HFileOutputFormat2 whose output is actually written by StoreFileWriter. It'll only be used in the intermediate output, so is safe to change between deploys.I chose to create a new ExtendedCellSerialization because it seemed the cleanest way to encapsulate, since others may have used CellSerialization in other contexts. The alternative would be to make CellSerialization implement Configurable, and use that to inject some new configuration which defaults to false. It felt better to skip needing to add a new configuration.
EDIT: I decided to wrap this in a conditional so its exposed to WALPlayer only for now. I realized that someone could have a job which reads Cells from some other sequence file they had created. A job configured by HFileOutputFormat2 would fail to read those if ExtendedCellSerialization were added. So adding to just WALPlayer preserves compatibility, since that job is end-to-end.