You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Joe Ferner <jo...@gmail.com> on 2014/03/07 20:28:52 UTC
Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/
-----------------------------------------------------------
Review request for accumulo.
Repository: accumulo
Description
-------
Add a NOT (!) operator to ColumnVisibility (ACCUMULO-2439)
Diffs
-----
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
Diff: https://reviews.apache.org/r/18917/diff/
Testing
-------
Thanks,
Joe Ferner
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/#review36565
-----------------------------------------------------------
Could you add what branch this diff is against? (Please make it against master, since this is a significant new feature)
Also please add a description of whatever testing you've already done.
- Sean Busbey
On March 7, 2014, 7:28 p.m., Joe Ferner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18917/
> -----------------------------------------------------------
>
> (Updated March 7, 2014, 7:28 p.m.)
>
>
> Review request for accumulo.
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Add a NOT (!) operator to ColumnVisibility (ACCUMULO-2439)
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
> core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
> core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
> core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
>
> Diff: https://reviews.apache.org/r/18917/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Joe Ferner
>
>
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Joe Ferner <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/
-----------------------------------------------------------
(Updated March 11, 2014, 3:51 p.m.)
Review request for accumulo.
Changes
-------
Add JavaDoc to inform user's that this will apply DeMorgan's.
Changed logic to switch statement.
Added double DeMorgan's test.
Repository: accumulo
Description
-------
This patch adds a NOT "!" operator to ColumnVisibility.
The syntax is as follows:
!a
(!a)&(!b)
a&(!b)
a&(!(b|c))
Because of the nature of the current visibility parsing algorithm the additional parentheses are required.
In the shell, the "Unable to render embedded object: File (" requires escaping. This is due to how JLine parses the command and attempts to substitute ") not found." with history.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
Diff: https://reviews.apache.org/r/18917/diff/
Testing
-------
This patch includes unit tests for parsing, flattening, and evaluating the not operator.
Thanks,
Joe Ferner
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/#review36568
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java
<https://reviews.apache.org/r/18917/#comment67572>
Add JavaDoc to inform user's that this will apply DeMorgan's.
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java
<https://reviews.apache.org/r/18917/#comment67568>
This would likely be better as a switch/case.
core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java
<https://reviews.apache.org/r/18917/#comment67570>
Can you add a double DeMorgan's test? i.e. start with "(!a|!b)&(!c|!d)" and normalize from there.
- Mike Drob
On March 7, 2014, 7:41 p.m., Joe Ferner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18917/
> -----------------------------------------------------------
>
> (Updated March 7, 2014, 7:41 p.m.)
>
>
> Review request for accumulo.
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This patch adds a NOT "!" operator to ColumnVisibility.
>
> The syntax is as follows:
> !a
> (!a)&(!b)
> a&(!b)
> a&(!(b|c))
>
> Because of the nature of the current visibility parsing algorithm the additional parentheses are required.
> In the shell, the "Unable to render embedded object: File (" requires escaping. This is due to how JLine parses the command and attempts to substitute ") not found." with history.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
> core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
> core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
> core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
>
> Diff: https://reviews.apache.org/r/18917/diff/
>
>
> Testing
> -------
>
> This patch includes unit tests for parsing, flattening, and evaluating the not operator.
>
>
> Thanks,
>
> Joe Ferner
>
>
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Jeff Kunkle <ku...@gmail.com>.
> On March 7, 2014, 9:19 p.m., Christopher Tubbs wrote:
> > The patch should make this feature optional. Preferably, make the visibility evaluation pluggable. At the very least, there's good reasons to disallow NOT terms, so this shouldn't be the default.
> >
> > I'd also like to see some additional tests, that read and write data to verify the feature end-to-end. This is changing some very important code that's been stable for some time, so more tests are always better.
>
> Jeff Kunkle wrote:
> What are the good reasons to disallow NOT terms? I'm not saying there aren't, I'm just not aware of them. I would greatly benefit from the NOT operator on a current project and hope this patch has a chance of being included.
>
> Josh Elser wrote:
> Jeff, I'd encourage you to read and participate in the discussion on the mailing lists as to the reasons behind this (http://mail-archives.apache.org/mod_mbox/accumulo-user/201403.mbox/%3C1394052507311-7949.post%40n5.nabble.com%3E). ReviewBoard isn't really the place to have this discussion (it's just not manageable).
Sorry. I posted here because someone commented on JIRA issue that it was easier to gather feedback here.
https://issues.apache.org/jira/browse/ACCUMULO-2439?focusedCommentId=13924236&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13924236
- Jeff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/#review36575
-----------------------------------------------------------
On March 11, 2014, 3:51 p.m., Joe Ferner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18917/
> -----------------------------------------------------------
>
> (Updated March 11, 2014, 3:51 p.m.)
>
>
> Review request for accumulo.
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This patch adds a NOT "!" operator to ColumnVisibility.
>
> The syntax is as follows:
> !a
> (!a)&(!b)
> a&(!b)
> a&(!(b|c))
>
> Because of the nature of the current visibility parsing algorithm the additional parentheses are required.
> In the shell, the "Unable to render embedded object: File (" requires escaping. This is due to how JLine parses the command and attempts to substitute ") not found." with history.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
> core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
> core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
> core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
>
> Diff: https://reviews.apache.org/r/18917/diff/
>
>
> Testing
> -------
>
> This patch includes unit tests for parsing, flattening, and evaluating the not operator.
>
>
> Thanks,
>
> Joe Ferner
>
>
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Josh Elser <jo...@gmail.com>.
> On March 7, 2014, 9:19 p.m., Christopher Tubbs wrote:
> > The patch should make this feature optional. Preferably, make the visibility evaluation pluggable. At the very least, there's good reasons to disallow NOT terms, so this shouldn't be the default.
> >
> > I'd also like to see some additional tests, that read and write data to verify the feature end-to-end. This is changing some very important code that's been stable for some time, so more tests are always better.
>
> Jeff Kunkle wrote:
> What are the good reasons to disallow NOT terms? I'm not saying there aren't, I'm just not aware of them. I would greatly benefit from the NOT operator on a current project and hope this patch has a chance of being included.
>
> Josh Elser wrote:
> Jeff, I'd encourage you to read and participate in the discussion on the mailing lists as to the reasons behind this (http://mail-archives.apache.org/mod_mbox/accumulo-user/201403.mbox/%3C1394052507311-7949.post%40n5.nabble.com%3E). ReviewBoard isn't really the place to have this discussion (it's just not manageable).
>
> Jeff Kunkle wrote:
> Sorry. I posted here because someone commented on JIRA issue that it was easier to gather feedback here.
>
> https://issues.apache.org/jira/browse/ACCUMULO-2439?focusedCommentId=13924236&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13924236
No worries -- Sean's comment was meant to gather feedback on the patch itself rather than Jira comments about certain lines in the patch.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/#review36575
-----------------------------------------------------------
On March 11, 2014, 3:51 p.m., Joe Ferner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18917/
> -----------------------------------------------------------
>
> (Updated March 11, 2014, 3:51 p.m.)
>
>
> Review request for accumulo.
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This patch adds a NOT "!" operator to ColumnVisibility.
>
> The syntax is as follows:
> !a
> (!a)&(!b)
> a&(!b)
> a&(!(b|c))
>
> Because of the nature of the current visibility parsing algorithm the additional parentheses are required.
> In the shell, the "Unable to render embedded object: File (" requires escaping. This is due to how JLine parses the command and attempts to substitute ") not found." with history.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
> core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
> core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
> core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
>
> Diff: https://reviews.apache.org/r/18917/diff/
>
>
> Testing
> -------
>
> This patch includes unit tests for parsing, flattening, and evaluating the not operator.
>
>
> Thanks,
>
> Joe Ferner
>
>
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Jeff Kunkle <ku...@gmail.com>.
> On March 7, 2014, 9:19 p.m., Christopher Tubbs wrote:
> > The patch should make this feature optional. Preferably, make the visibility evaluation pluggable. At the very least, there's good reasons to disallow NOT terms, so this shouldn't be the default.
> >
> > I'd also like to see some additional tests, that read and write data to verify the feature end-to-end. This is changing some very important code that's been stable for some time, so more tests are always better.
What are the good reasons to disallow NOT terms? I'm not saying there aren't, I'm just not aware of them. I would greatly benefit from the NOT operator on a current project and hope this patch has a chance of being included.
- Jeff
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/#review36575
-----------------------------------------------------------
On March 11, 2014, 3:51 p.m., Joe Ferner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18917/
> -----------------------------------------------------------
>
> (Updated March 11, 2014, 3:51 p.m.)
>
>
> Review request for accumulo.
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This patch adds a NOT "!" operator to ColumnVisibility.
>
> The syntax is as follows:
> !a
> (!a)&(!b)
> a&(!b)
> a&(!(b|c))
>
> Because of the nature of the current visibility parsing algorithm the additional parentheses are required.
> In the shell, the "Unable to render embedded object: File (" requires escaping. This is due to how JLine parses the command and attempts to substitute ") not found." with history.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
> core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
> core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
> core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
>
> Diff: https://reviews.apache.org/r/18917/diff/
>
>
> Testing
> -------
>
> This patch includes unit tests for parsing, flattening, and evaluating the not operator.
>
>
> Thanks,
>
> Joe Ferner
>
>
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Josh Elser <jo...@gmail.com>.
> On March 7, 2014, 9:19 p.m., Christopher Tubbs wrote:
> > The patch should make this feature optional. Preferably, make the visibility evaluation pluggable. At the very least, there's good reasons to disallow NOT terms, so this shouldn't be the default.
> >
> > I'd also like to see some additional tests, that read and write data to verify the feature end-to-end. This is changing some very important code that's been stable for some time, so more tests are always better.
>
> Jeff Kunkle wrote:
> What are the good reasons to disallow NOT terms? I'm not saying there aren't, I'm just not aware of them. I would greatly benefit from the NOT operator on a current project and hope this patch has a chance of being included.
Jeff, I'd encourage you to read and participate in the discussion on the mailing lists as to the reasons behind this (http://mail-archives.apache.org/mod_mbox/accumulo-user/201403.mbox/%3C1394052507311-7949.post%40n5.nabble.com%3E). ReviewBoard isn't really the place to have this discussion (it's just not manageable).
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/#review36575
-----------------------------------------------------------
On March 11, 2014, 3:51 p.m., Joe Ferner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18917/
> -----------------------------------------------------------
>
> (Updated March 11, 2014, 3:51 p.m.)
>
>
> Review request for accumulo.
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This patch adds a NOT "!" operator to ColumnVisibility.
>
> The syntax is as follows:
> !a
> (!a)&(!b)
> a&(!b)
> a&(!(b|c))
>
> Because of the nature of the current visibility parsing algorithm the additional parentheses are required.
> In the shell, the "Unable to render embedded object: File (" requires escaping. This is due to how JLine parses the command and attempts to substitute ") not found." with history.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
> core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
> core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
> core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
>
> Diff: https://reviews.apache.org/r/18917/diff/
>
>
> Testing
> -------
>
> This patch includes unit tests for parsing, flattening, and evaluating the not operator.
>
>
> Thanks,
>
> Joe Ferner
>
>
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/#review36575
-----------------------------------------------------------
The patch should make this feature optional. Preferably, make the visibility evaluation pluggable. At the very least, there's good reasons to disallow NOT terms, so this shouldn't be the default.
I'd also like to see some additional tests, that read and write data to verify the feature end-to-end. This is changing some very important code that's been stable for some time, so more tests are always better.
- Christopher Tubbs
On March 7, 2014, 2:41 p.m., Joe Ferner wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/18917/
> -----------------------------------------------------------
>
> (Updated March 7, 2014, 2:41 p.m.)
>
>
> Review request for accumulo.
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> This patch adds a NOT "!" operator to ColumnVisibility.
>
> The syntax is as follows:
> !a
> (!a)&(!b)
> a&(!b)
> a&(!(b|c))
>
> Because of the nature of the current visibility parsing algorithm the additional parentheses are required.
> In the shell, the "Unable to render embedded object: File (" requires escaping. This is due to how JLine parses the command and attempts to substitute ") not found." with history.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
> core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
> core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
> core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
>
> Diff: https://reviews.apache.org/r/18917/diff/
>
>
> Testing
> -------
>
> This patch includes unit tests for parsing, flattening, and evaluating the not operator.
>
>
> Thanks,
>
> Joe Ferner
>
>
Re: Review Request 18917: Add a NOT (!) operator to ColumnVisibility
(ACCUMULO-2439)
Posted by Joe Ferner <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/18917/
-----------------------------------------------------------
(Updated March 7, 2014, 7:41 p.m.)
Review request for accumulo.
Repository: accumulo
Description (updated)
-------
This patch adds a NOT "!" operator to ColumnVisibility.
The syntax is as follows:
!a
(!a)&(!b)
a&(!b)
a&(!(b|c))
Because of the nature of the current visibility parsing algorithm the additional parentheses are required.
In the shell, the "Unable to render embedded object: File (" requires escaping. This is due to how JLine parses the command and attempts to substitute ") not found." with history.
Diffs
-----
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java 75091d2
core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java 725b2c7
core/src/test/java/org/apache/accumulo/core/security/ColumnVisibilityTest.java 7a6a80d
core/src/test/java/org/apache/accumulo/core/security/VisibilityEvaluatorTest.java ee4d2ee
Diff: https://reviews.apache.org/r/18917/diff/
Testing (updated)
-------
This patch includes unit tests for parsing, flattening, and evaluating the not operator.
Thanks,
Joe Ferner