[HBASE-25246] Backup/Restore hbase cell tags. #2745
Closed
Conversation
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
|
🎊 +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. |
Contributor
Author
|
@anoopsjohn @virajjasani Could you please provide your feedback. Thank you !! |
Contributor
Author
|
Thank you @anoopsjohn and @virajjasani for your offline help. Could you guys please review the changes. Thank you ! |
|
🎊 +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. |
This was referenced Dec 10, 2020
anoopsjohn
approved these changes
Dec 12, 2020
Contributor
anoopsjohn
left a comment
anoopsjohn
left a comment
There was a problem hiding this comment.
Looks good. Added few minor suggestions.
Can u also add a jira to check abt other tools? There is another way of Export tool right which uses CPEP. Also CopyTable should make sure copying tags.
| ProtobufUtil.toCell(cell, false); | ||
| assertNotNull(protoCell); | ||
|
|
||
| Cell decodedCell = getCellFromProtoResult(protoCell, false); |
Contributor
There was a problem hiding this comment.
Can you also add getCellFromProtoResult(protoCell, true ); and assert not tags come back. This clearly assert when POJO is encoded to PB object no tags been encoded.
Contributor
Author
There was a problem hiding this comment.
The boolean parameter in getCellFromProtoResult method is whether to encode tags or not. If that boolean (encodeTags) is set to true then response will return tags if it contains tags.
If encodeTags is set to false, then response will not return tags even if the cell contains tags.
I have 2 tests testCellConversionWithoutTags and testCellConversionWithTags which tests the above 2 scenarios.
For both of the tests the cell contains 1 tags.
testCellConversionWithoutTags method will set the encodeTags boolean to false and verifies whether response doesn't contain tags.
testCellConversionWithTags method will set the encodeTags boolean to true and verifies whether response does contain tags.
@anoopsjohn Does that answer your question or maybe I am not able to understand the question properly. Please elaborate if its the latter case. Thank you !
Contributor
There was a problem hiding this comment.
One test is passing true while doing encoding and then passing true while decoding and make sure tags come back
The other pass both as false and make sure no tags present in final decoded cell. Good
What my request was to have a test case where the encode time will say false. And then decode call pass true(if any tags in serialized form decode that) and assert still no tags are present in final decoded Cell. The second test case will pass even if tomorrow the encode logic changed to serialize tags always (which we dont want to happen). So the new test case really assert that while encoding if boolean param is false its not really serializing the tags. You follow now? Sorry for not being clear in my earlier comment
Contributor
Author
There was a problem hiding this comment.
@anoopsjohn Makes sense. Added a new test case as per your feedback. Thank you !
Contributor
There was a problem hiding this comment.
It seems we have good coverage, maybe one more possibility: encode with true and decode with false should also result in no tags available in decoded cell. Not blocker but good to have this test?
| String[] args = new String[] { | ||
| "-D" + ExportUtils.RAW_SCAN + "=true", | ||
| // This will make sure that codec will encode and decode tags in rpc call. | ||
| "-Dhbase.client.rpc.codec=org.apache.hadoop.hbase.codec.KeyValueCodecWithTags", |
Contributor
There was a problem hiding this comment.
If we have entry in book regarding Export tool, we can add this detail also there about changing Codec. Can be done in another issue too.
| d.setAttribute(TEST_ATTR, Bytes.toBytes(TEST_TAG)); | ||
| exportT.delete(d); | ||
|
|
||
| // Run export too with KeyValueCodecWithTags as Codec. This will ensure that export tool |
|
🎊 +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. |
Contributor
|
This seems relevant? https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2745/6/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-root.txt |
virajjasani
approved these changes
Dec 14, 2020
Contributor
virajjasani
left a comment
virajjasani
left a comment
There was a problem hiding this comment.
+1, left one comment.
| ProtobufUtil.toCell(cell, false); | ||
| assertNotNull(protoCell); | ||
|
|
||
| Cell decodedCell = getCellFromProtoResult(protoCell, false); |
Contributor
There was a problem hiding this comment.
It seems we have good coverage, maybe one more possibility: encode with true and decode with false should also result in no tags available in decoded cell. Not blocker but good to have this test?
Contributor
Author
|
Failed test TestReplicationEndpointWithMultipleAsyncWAL ran successfully locally. @virajjasani Added test case according to your feedback. Thank you ! |
Contributor
|
Thanks @shahrs87 , after QA results, this is good to go. In the meanwhile, you can keep branch-2 and branch-1 ready with latest changes. |
|
🎊 +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. |
Contributor
|
@shahrs87 https://ci-hadoop.apache.org/job/HBase/job/HBase-PreCommit-GitHub-PR/job/PR-2745/9/testReport/ Can you once confirm locally? Unless quite many changes required, we can merge this soon anyways. |
Contributor
Author
|
TestIncrementalBackupWithBulkLoad succeeds locally. |
|
🎊 +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. |
virajjasani
pushed a commit
that referenced
this pull request
Dec 16, 2020
Closes #2745 Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Dec 16, 2020
Closes #2745 Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Dec 17, 2020
Closes #2745 Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Dec 17, 2020
Closes #2745 Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
virajjasani
pushed a commit
that referenced
this pull request
Dec 17, 2020
Closes #2745 Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
wchevreuil
pushed a commit
to wchevreuil/hbase
that referenced
this pull request
May 24, 2021
Closes apache#2745 Signed-off-by: Anoop Sam John <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> (cherry picked from commit b1a1d47) Change-Id: Ie90d6b3182c9097e4760ab34aef2311ba483a64e
Sign up for free
to 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.
@anoopsjohn @virajjasani I have added a boolean flag (encodeTags/decodeTags) in ProtobufUtil which will evaluate to true if client cell block support is enabled.
Could you please tell me if I am thinking in the right direction ? Thank you !