[ 
https://issues.apache.org/jira/browse/HBASE-10885?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14020626#comment-14020626
 ] 

Andrew Purtell commented on HBASE-10885:
----------------------------------------

Test results look good.

Minor stuff.

{code}
diff --git hbase-common/src/main/java/org/apache/hadoop/hbase/TagType.java 
hbase-common/src/main/java/org/apache/hadoop/hbase/TagType.java
index 6c43c78..6e79038 100644
--- hbase-common/src/main/java/org/apache/hadoop/hbase/TagType.java
+++ hbase-common/src/main/java/org/apache/hadoop/hbase/TagType.java
@@ -27,4 +27,5 @@ public final class TagType {
   public static final byte ACL_TAG_TYPE = (byte) 1;
   public static final byte VISIBILITY_TAG_TYPE = (byte) 2;
   public static final byte LOG_REPLAY_TAG_TYPE = (byte) 3;
+  public static final byte VISBILITY_EXP_SERIALIZATION_FORMAT = (byte)4;
{code}

Misspelled. Also, can we use a better name for this? Everything else is named 
foo_TAG_TYPE.

When you write that new tag e.g. in VisibilityController#createVisibilityTags 
you are doing this:

{code}
+    // Add new tag type
+    tags.add(new Tag(VisibilityUtils.VISBILITY_EXP_SERIALIZATION_FORMAT,
+        HConstants.EMPTY_BYTE_ARRAY));
{code}

The tag is empty (length 0) - doesn't seem right. Later checks for whether the 
serialization format has sorted ordinals or not use the presence of this tag as 
definitive, e.g.

{code}
+      if(tag.getType() == VisibilityUtils.VISBILITY_EXP_SERIALIZATION_FORMAT) {
{code}

That doesn't seem right either. The intent is versioning the serialization 
format. Therefore let's version it, with an integer value, and test the value. 

Comments in VisibilityScanDeleteTracker are good.

Many unit tests, representing the bulk of the patch, good.

bq. Tested the patch with an IT case that am working on internally. Will also 
add the IT patch once it is tested fully. 

But not in the v2 patch, right? So that will be in v3?

> Support visibility expressions on Deletes
> -----------------------------------------
>
>                 Key: HBASE-10885
>                 URL: https://issues.apache.org/jira/browse/HBASE-10885
>             Project: HBase
>          Issue Type: Improvement
>    Affects Versions: 0.98.1
>            Reporter: Andrew Purtell
>            Assignee: ramkrishna.s.vasudevan
>            Priority: Blocker
>             Fix For: 0.99.0, 0.98.4
>
>         Attachments: HBASE-10885_1.patch, HBASE-10885_2.patch, 
> HBASE-10885_new_tag_type_1.patch, HBASE-10885_new_tag_type_2.patch, 
> HBASE-10885_v1.patch, HBASE-10885_v2.patch, HBASE-10885_v2.patch, 
> HBASE-10885_v2.patch
>
>
> Accumulo can specify visibility expressions for delete markers. During 
> compaction the cells covered by the tombstone are determined in part by 
> matching the visibility expression. This is useful for the use case of data 
> set coalescing, where entries from multiple data sets carrying different 
> labels are combined into one common large table. Later, a subset of entries 
> can be conveniently removed using visibility expressions.
> Currently doing the same in HBase would only be possible with a custom 
> coprocessor. Otherwise, a Delete will affect all cells covered by the 
> tombstone regardless of any visibility expression scoping. This is correct 
> behavior in that no data spill is possible, but certainly could be 
> surprising, and is only meant to be transitional. We decided not to support 
> visibility expressions on Deletes to control the complexity of the initial 
> implementation.



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to