Discussion: Support adding in an accumulo-access expression to a column visibility #5293
Conversation
ddanielr
force-pushed
the
feature/support-accumulo-access
branch
from
16c43a9 to
9cd07f5
Compare
| */ | ||
| public ColumnVisibility(AccessExpression expression) { | ||
| // AccessExpression is a validated immutable object, so no need to re validate | ||
| this.expression = expression.getExpression().getBytes(UTF_8); |
There was a problem hiding this comment.
Is this just an optimization, or does calling validate cause an issue? A client could use AccessExpression's and then create a ColumnVisibilty using the byte[] constructor.
There was a problem hiding this comment.
This is an optimization to avoid calling validate for performance reasons. If an AccessExpression is passed in then it has already performed a validate operation previous and does not need to parse the expression.
There was a problem hiding this comment.
We could add a constructor (byte[]) then that doesn't perform validation, mark it deprecated, and in the javadoc:
- that this constructor was added for clients that want to use Accumulo-Access before Accumulo 4.0
- this will be removed in 4.0
- using this incorrectly (with an invalid column visibility) will likely cause exceptions and no data to be returned
There was a problem hiding this comment.
I'm thinking that the above approach may be a good compromise to allow Accumulo-Access to be used with 2.1 without introducing the dependency in a patch release.
There was a problem hiding this comment.
I like that approach.
There was a problem hiding this comment.
Considering that we already have a ColumnVisibility(byte[]) constructor, we would likely need to create a ColumnVisibility(byte[], boolean) constructor.
There was a problem hiding this comment.
Considering that we already have a
ColumnVisibility(byte[])constructor, we would likely need to create aColumnVisibility(byte[], boolean)constructor.
I added that constructor but made it private. Then I added a new static method to use the private constructor called fromAccessExpression so that creation of these unvalidated ColumnVisibilities is very explicit.
public static ColumnVisibility fromAccessExpression(byte[] incomingExpression) {
return new ColumnVisibility(incomingExpression, false);
}
The name should probably be changed, so I'm open to suggestions.
Adds a new constructor that supports an AccessExpression. Also adds deep copy method for existing ColumnVisibilities.
ddanielr
force-pushed
the
feature/support-accumulo-access
branch
from
9cd07f5 to
1645738
Compare
Removes accumulo-access and adds new private constructor that allows validation to be skipped. Created new static method to generate an unvalidated ColumnVisibility
| * @param expression visibility expression, encoded as UTF-8 bytes | ||
| * @param validate enables or disables validation and parse tree generation | ||
| */ | ||
| private ColumnVisibility(byte[] expression, boolean validate) { |
There was a problem hiding this comment.
need to change the validate variable name to avoid conflicting with the validate method
| public ColumnVisibility(ColumnVisibility visibility) { | ||
| byte[] incomingExpression = visibility.expression; | ||
| this.expression = Arrays.copyOf(incomingExpression, incomingExpression.length); | ||
| this.node = new Node(visibility.node); |
There was a problem hiding this comment.
This constructor is still needed since it also copies the parse tree from the original ColVis
There was a problem hiding this comment.
I think there is an issue here with the parse tree being mutable. Given that ColumnVisibility's can be cached, and that the parse tree is mutable, it's possible that the parse tree could be mutating in another thread while this copy constructor is called. I'm not sure how to resolve this issue. Using the Cloneable interface and adding a clone method doesn't fully solve this issue either. I think this issue exists in #5217 as well.
There was a problem hiding this comment.
It's possible that doing the following may help the issue I raised above:
- Modify
Node.childrento beCollections.synchronizedList(new ArrayList<>())inNode.add - synchronize on visibility.node in the copy constructor
| @@ -238,6 +239,15 @@ public void testParseTreesOrdering() { | |||
| assertTrue(flat.indexOf('b') < flat.indexOf('a'), "shortest children sort first"); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Need to add test cases for new constructors.
| validate(expression); | ||
| } else { | ||
| this.expression = Arrays.copyOf(expression, expression.length); | ||
| this.node = EMPTY_NODE; |
There was a problem hiding this comment.
I think the expression needs to be parsed to populate node otherwise we run into issues in other areas of the code. For example, here in VisibilityEvaluator an Exception will be thrown. Here in NodeComparator all ColumnVisibilities that are created this way will be "equal".
| public ColumnVisibility(ColumnVisibility visibility) { | ||
| byte[] incomingExpression = visibility.expression; | ||
| this.expression = Arrays.copyOf(incomingExpression, incomingExpression.length); | ||
| this.node = new Node(visibility.node); |
There was a problem hiding this comment.
I think there is an issue here with the parse tree being mutable. Given that ColumnVisibility's can be cached, and that the parse tree is mutable, it's possible that the parse tree could be mutating in another thread while this copy constructor is called. I'm not sure how to resolve this issue. Using the Cloneable interface and adding a clone method doesn't fully solve this issue either. I think this issue exists in #5217 as well.
Includes changes from #5217
This PR adds a new dependency to 2.1.4 which I'm not sure we want to do.
Using this PR to evaluate ColumnVisibility usages in 2.1 to think through performance concerns.
Merging this requires conversation about if we want to do this in 2.1 vs maybe doing a 2.2 release to use as a stepping stone.