Skip to content

[HBASE-25246] Backup/Restore hbase cell tags. #2745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shahrs87 wants to merge 7 commits into from
Closed

[HBASE-25246] Backup/Restore hbase cell tags. #2745

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7937bbc
[HBASE-25246] Backup/Restore hbase cell tags.
Dec 9, 2020
37ce84c
Merge branch 'master' of https://github.com/apache/hbase into HBASE-2…
Dec 10, 2020
bd45753
[HBASE-25246] Add review comments
Dec 10, 2020
443178a
Merge branch 'master' of https://github.com/apache/hbase into HBASE-2…
Dec 12, 2020
1554a3b
Addressing review comments
Dec 12, 2020
8c7cc5e
Addressing review comments
Dec 13, 2020
a56e97b
Addressing code review comments
Dec 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 56 additions & 14 deletions hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/ProtobufUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,6 +1436,21 @@ private static MutationProto.Builder getMutationBuilderAndSetCommonFields(final
* @return the converted protocol buffer Result
*/
public static ClientProtos.Result toResult(final Result result) {
return toResult(result, false);
}

/**
* Convert a client Result to a protocol buffer Result
* @param result the client Result to convert
* @param encodeTags whether to includeTags in converted protobuf result or not
* When @encodeTags is set to true, it will return all the tags in the response.
* These tags may contain some sensitive data like acl permissions, etc.
* Only the tools like Export, Import which needs to take backup needs to set
* it to true so that cell tags are persisted in backup.
* Refer to HBASE-25246 for more context.
* @return the converted protocol buffer Result
*/
public static ClientProtos.Result toResult(final Result result, boolean encodeTags) {
if (result.getExists() != null) {
return toResult(result.getExists(), result.isStale());
}
Expand All @@ -1447,7 +1462,7 @@ public static ClientProtos.Result toResult(final Result result) {

ClientProtos.Result.Builder builder = ClientProtos.Result.newBuilder();
for (Cell c : cells) {
builder.addCell(toCell(c));
builder.addCell(toCell(c, encodeTags));
}

builder.setStale(result.isStale());
Expand Down Expand Up @@ -1494,6 +1509,22 @@ public static ClientProtos.Result toResultNoData(final Result result) {
* @return the converted client Result
*/
public static Result toResult(final ClientProtos.Result proto) {
return toResult(proto, false);
}

/**
* Convert a protocol buffer Result to a client Result
*
* @param proto the protocol buffer Result to convert
* @param decodeTags whether to decode tags into converted client Result
* When @decodeTags is set to true, it will decode all the tags from the
* response. These tags may contain some sensitive data like acl permissions,
* etc. Only the tools like Export, Import which needs to take backup needs to
* set it to true so that cell tags are persisted in backup.
* Refer to HBASE-25246 for more context.
* @return the converted client Result
*/
public static Result toResult(final ClientProtos.Result proto, boolean decodeTags) {
if (proto.hasExists()) {
if (proto.getStale()) {
return proto.getExists() ? EMPTY_RESULT_EXISTS_TRUE_STALE :EMPTY_RESULT_EXISTS_FALSE_STALE;
Expand All @@ -1509,7 +1540,7 @@ public static Result toResult(final ClientProtos.Result proto) {
List<Cell> cells = new ArrayList<>(values.size());
ExtendedCellBuilder builder = ExtendedCellBuilderFactory.create(CellBuilderType.SHALLOW_COPY);
for (CellProtos.Cell c : values) {
cells.add(toCell(builder, c));
cells.add(toCell(builder, c, decodeTags));
}
return Result.create(cells, null, proto.getStale(), proto.getPartial());
}
Expand Down Expand Up @@ -1552,7 +1583,7 @@ public static Result toResult(final ClientProtos.Result proto, final CellScanner
if (cells == null) cells = new ArrayList<>(values.size());
ExtendedCellBuilder builder = ExtendedCellBuilderFactory.create(CellBuilderType.SHALLOW_COPY);
for (CellProtos.Cell c: values) {
cells.add(toCell(builder, c));
cells.add(toCell(builder, c, false));
}
}

Expand Down Expand Up @@ -2000,7 +2031,7 @@ public static void toIOException(ServiceException se) throws IOException {
throw new IOException(se);
}

public static CellProtos.Cell toCell(final Cell kv) {
public static CellProtos.Cell toCell(final Cell kv, boolean encodeTags) {
// Doing this is going to kill us if we do it for all data passed.
// St.Ack 20121205
CellProtos.Cell.Builder kvbuilder = CellProtos.Cell.newBuilder();
Expand All @@ -2015,7 +2046,10 @@ public static CellProtos.Cell toCell(final Cell kv) {
kvbuilder.setTimestamp(kv.getTimestamp());
kvbuilder.setValue(wrap(((ByteBufferExtendedCell) kv).getValueByteBuffer(),
((ByteBufferExtendedCell) kv).getValuePosition(), kv.getValueLength()));
// TODO : Once tags become first class then we may have to set tags to kvbuilder.
if (encodeTags) {
kvbuilder.setTags(wrap(((ByteBufferExtendedCell) kv).getTagsByteBuffer(),
((ByteBufferExtendedCell) kv).getTagsPosition(), kv.getTagsLength()));
}
} else {
kvbuilder.setRow(
UnsafeByteOperations.unsafeWrap(kv.getRowArray(), kv.getRowOffset(), kv.getRowLength()));
Expand All @@ -2027,6 +2061,10 @@ public static CellProtos.Cell toCell(final Cell kv) {
kvbuilder.setTimestamp(kv.getTimestamp());
kvbuilder.setValue(UnsafeByteOperations.unsafeWrap(kv.getValueArray(), kv.getValueOffset(),
kv.getValueLength()));
if (encodeTags) {
kvbuilder.setTags(UnsafeByteOperations.unsafeWrap(kv.getTagsArray(), kv.getTagsOffset(),
kv.getTagsLength()));
}
}
return kvbuilder.build();
}
Expand All @@ -2038,15 +2076,19 @@ private static ByteString wrap(ByteBuffer b, int offset, int length) {
return UnsafeByteOperations.unsafeWrap(dup);
}

public static Cell toCell(ExtendedCellBuilder cellBuilder, final CellProtos.Cell cell) {
return cellBuilder.clear()
.setRow(cell.getRow().toByteArray())
.setFamily(cell.getFamily().toByteArray())
.setQualifier(cell.getQualifier().toByteArray())
.setTimestamp(cell.getTimestamp())
.setType((byte) cell.getCellType().getNumber())
.setValue(cell.getValue().toByteArray())
.build();
public static Cell toCell(ExtendedCellBuilder cellBuilder, final CellProtos.Cell cell,
boolean decodeTags) {
ExtendedCellBuilder builder = cellBuilder.clear()
.setRow(cell.getRow().toByteArray())
.setFamily(cell.getFamily().toByteArray())
.setQualifier(cell.getQualifier().toByteArray())
.setTimestamp(cell.getTimestamp())
.setType((byte) cell.getCellType().getNumber())
.setValue(cell.getValue().toByteArray());
if (decodeTags && cell.hasTags()) {
builder.setTags(cell.getTags().toByteArray());
}
return builder.build();
}

public static HBaseProtos.NamespaceDescriptor toProtoNamespaceDescriptor(NamespaceDescriptor ns) {
Expand Down
103 changes: 100 additions & 3 deletions hbase-client/src/test/java/org/apache/hadoop/hbase/shaded/protobuf/TestProtobufUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,17 +18,24 @@
package org.apache.hadoop.hbase.shaded.protobuf;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

import java.io.IOException;
import java.nio.ByteBuffer;
import java.util.Collections;
import java.util.List;
import org.apache.hadoop.hbase.ArrayBackedTag;
import org.apache.hadoop.hbase.ByteBufferKeyValue;
import org.apache.hadoop.hbase.Cell;
import org.apache.hadoop.hbase.CellBuilderType;
import org.apache.hadoop.hbase.CellComparatorImpl;
import org.apache.hadoop.hbase.ExtendedCellBuilder;
import org.apache.hadoop.hbase.ExtendedCellBuilderFactory;
import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.KeyValue;
import org.apache.hadoop.hbase.PrivateCellUtil;
import org.apache.hadoop.hbase.Tag;
import org.apache.hadoop.hbase.client.Append;
import org.apache.hadoop.hbase.client.Delete;
import org.apache.hadoop.hbase.client.Get;
Expand Down Expand Up @@ -63,7 +70,8 @@ public class TestProtobufUtil {
@ClassRule
public static final HBaseClassTestRule CLASS_RULE =
HBaseClassTestRule.forClass(TestProtobufUtil.class);

private static final String TAG_STR = "tag-1";
private static final byte TAG_TYPE = (byte)10;
public TestProtobufUtil() {
}

Expand Down Expand Up @@ -271,9 +279,10 @@ public void testToCell() {
ByteBuffer dbb = ByteBuffer.allocateDirect(arr.length);
dbb.put(arr);
ByteBufferKeyValue offheapKV = new ByteBufferKeyValue(dbb, kv1.getLength(), kv2.getLength());
CellProtos.Cell cell = ProtobufUtil.toCell(offheapKV);
CellProtos.Cell cell = ProtobufUtil.toCell(offheapKV, false);
Cell newOffheapKV =
ProtobufUtil.toCell(ExtendedCellBuilderFactory.create(CellBuilderType.SHALLOW_COPY), cell);
ProtobufUtil.toCell(ExtendedCellBuilderFactory.create(CellBuilderType.SHALLOW_COPY), cell,
false);
assertTrue(CellComparatorImpl.COMPARATOR.compare(offheapKV, newOffheapKV) == 0);
}

Expand Down Expand Up @@ -479,4 +488,92 @@ public void testRegionLockInfo() {
+ "\"sharedLockCount\":0"
+ "}]", lockJson);
}

/**
* Test {@link ProtobufUtil#toCell(Cell, boolean)} and
* {@link ProtobufUtil#toCell(ExtendedCellBuilder, CellProtos.Cell, boolean)} conversion
* methods when it contains tags and encode/decode tags is set to true.
*/
@Test
public void testCellConversionWithTags() {

Cell cell = getCellWithTags();
CellProtos.Cell protoCell = ProtobufUtil.toCell(cell, true);
assertNotNull(protoCell);

Cell decodedCell = getCellFromProtoResult(protoCell, true);
List<Tag> decodedTags = PrivateCellUtil.getTags(decodedCell);
assertEquals(1, decodedTags.size());
Tag decodedTag = decodedTags.get(0);
assertEquals(TAG_TYPE, decodedTag.getType());
assertEquals(TAG_STR, Tag.getValueAsString(decodedTag));
}

private Cell getCellWithTags() {
Tag tag = new ArrayBackedTag(TAG_TYPE, TAG_STR);
ExtendedCellBuilder cellBuilder = ExtendedCellBuilderFactory.create(CellBuilderType.DEEP_COPY);
cellBuilder.setRow(Bytes.toBytes("row1"));
cellBuilder.setFamily(Bytes.toBytes("f1"));
cellBuilder.setQualifier(Bytes.toBytes("q1"));
cellBuilder.setValue(Bytes.toBytes("value1"));
cellBuilder.setType(Cell.Type.Delete);
cellBuilder.setTags(Collections.singletonList(tag));
return cellBuilder.build();
}

private Cell getCellFromProtoResult(CellProtos.Cell protoCell, boolean decodeTags) {
ExtendedCellBuilder decodedBuilder =
ExtendedCellBuilderFactory.create(CellBuilderType.DEEP_COPY);
return ProtobufUtil.toCell(decodedBuilder, protoCell, decodeTags);
}

/**
* Test {@link ProtobufUtil#toCell(Cell, boolean)} and
* {@link ProtobufUtil#toCell(ExtendedCellBuilder, CellProtos.Cell, boolean)} conversion
* methods when it contains tags and encode/decode tags is set to false.
*/
@Test
public void testCellConversionWithoutTags() {
Cell cell = getCellWithTags();
CellProtos.Cell protoCell = ProtobufUtil.toCell(cell, false);
assertNotNull(protoCell);

Cell decodedCell = getCellFromProtoResult(protoCell, false);
List<Tag> decodedTags = PrivateCellUtil.getTags(decodedCell);
assertEquals(0, decodedTags.size());
}

/**
* Test {@link ProtobufUtil#toCell(Cell, boolean)} and
* {@link ProtobufUtil#toCell(ExtendedCellBuilder, CellProtos.Cell, boolean)} conversion
* methods when it contains tags and encoding of tags is set to false
* and decoding of tags is set to true.
*/
@Test
public void testTagEncodeFalseDecodeTrue() {
Cell cell = getCellWithTags();
CellProtos.Cell protoCell = ProtobufUtil.toCell(cell, false);
assertNotNull(protoCell);

Cell decodedCell = getCellFromProtoResult(protoCell, true);
List<Tag> decodedTags = PrivateCellUtil.getTags(decodedCell);
assertEquals(0, decodedTags.size());
}

/**
* Test {@link ProtobufUtil#toCell(Cell, boolean)} and
* {@link ProtobufUtil#toCell(ExtendedCellBuilder, CellProtos.Cell, boolean)} conversion
* methods when it contains tags and encoding of tags is set to true
* and decoding of tags is set to false.
*/
@Test
public void testTagEncodeTrueDecodeFalse() {
Cell cell = getCellWithTags();
CellProtos.Cell protoCell = ProtobufUtil.toCell(cell, true);
assertNotNull(protoCell);

Cell decodedCell = getCellFromProtoResult(protoCell, false);
Copy link
Contributor

@anoopsjohn anoopsjohn Dec 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@shahrs87 shahrs87 Dec 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 !

Copy link
Contributor

@anoopsjohn anoopsjohn Dec 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@shahrs87 shahrs87 Dec 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anoopsjohn Makes sense. Added a new test case as per your feedback. Thank you !

Copy link
Contributor

@virajjasani virajjasani Dec 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

List<Tag> decodedTags = PrivateCellUtil.getTags(decodedCell);
assertEquals(0, decodedTags.size());
}
}
5 changes: 4 additions & 1 deletion hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/Import.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
import org.apache.hadoop.hbase.KeyValue;
import org.apache.hadoop.hbase.KeyValueUtil;
import org.apache.hadoop.hbase.TableName;
import org.apache.hadoop.hbase.Tag;
import org.apache.hadoop.hbase.ZooKeeperConnectionException;
import org.apache.hadoop.hbase.util.MapReduceExtendedCell;
import org.apache.hadoop.hbase.zookeeper.ZKWatcher;
Expand Down Expand Up @@ -511,6 +512,7 @@ private static Cell convertKv(Cell kv, Map<byte[], byte[]> cfRenameMap) {
// If there's a rename mapping for this CF, create a new KeyValue
byte[] newCfName = cfRenameMap.get(CellUtil.cloneFamily(kv));
if (newCfName != null) {
List<Tag> tags = PrivateCellUtil.getTags(kv);
kv = new KeyValue(kv.getRowArray(), // row buffer
kv.getRowOffset(), // row offset
kv.getRowLength(), // row length
Expand All @@ -524,7 +526,8 @@ private static Cell convertKv(Cell kv, Map<byte[], byte[]> cfRenameMap) {
KeyValue.Type.codeToType(kv.getTypeByte()), // KV Type
kv.getValueArray(), // value buffer
kv.getValueOffset(), // value offset
kv.getValueLength()); // value length
kv.getValueLength(), // value length
tags.size() == 0 ? null: tags);
}
}
return kv;
Expand Down
4 changes: 2 additions & 2 deletions hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/ResultSerialization.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ public void close() throws IOException {
@Override
public Result deserialize(Result mutation) throws IOException {
ClientProtos.Result proto = ClientProtos.Result.parseDelimitedFrom(in);
return ProtobufUtil.toResult(proto);
return ProtobufUtil.toResult(proto, true);
}

@Override
Expand All @@ -152,7 +152,7 @@ public void open(OutputStream out) throws IOException {

@Override
public void serialize(Result result) throws IOException {
ProtobufUtil.toResult(result).writeDelimitedTo(out);
ProtobufUtil.toResult(result, true).writeDelimitedTo(out);
}
}
}
Loading