You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@accumulo.apache.org by Corey Nolet <cj...@gmail.com> on 2014/12/31 05:05:40 UTC

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/
-----------------------------------------------------------

(Updated Dec. 31, 2014, 4:05 a.m.)


Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.


Changes
-------

Added accumulo group to review.


Repository: accumulo


Description
-------

ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.


Diffs
-----

  core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
  core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
  core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
  core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
  core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
  core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
  core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
  core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
  core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
  server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
  server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
  test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
  test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29502/diff/


Testing
-------

Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment


Thanks,

Corey Nolet

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.


> On Dec. 31, 2014, 4:30 a.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java, line 197
> > <https://reviews.apache.org/r/29502/diff/1/?file=804415#file804415line197>
> >
> >     Can't you pull this from the Scanner?

I didn't see a good way to get this info from the scanner. The more I think about this- a simple getter on the scanner would be massively useful.


> On Dec. 31, 2014, 4:30 a.m., Josh Elser wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java, line 55
> > <https://reviews.apache.org/r/29502/diff/1/?file=804426#file804426line55>
> >
> >     It looks like TabletIteratorEnvironment is used for minor compactions. Isn't always setting `Authorizations.EMPTY` a little misleading? Is there something more representative of having all auths we could do here? Maybe extra documentation is enough? Could also throw UnsupportedOperationException or similar when the IteratorScope is something that isn't SCAN?

Good point! This should definitely be documented as a scan-time only operation. I'm on the fence about throwing an exception- I think I could go either way on that.


> On Dec. 31, 2014, 4:30 a.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java, line 54
> > <https://reviews.apache.org/r/29502/diff/1/?file=804430#file804430line54>
> >
> >     Please create a user, assign it the auths you need, and then remove the user after the test.
> >     
> >     If this test is run against a standalone instance, it should try to leave the system in the same state the test started in.

You know I was thinking about this when I was coding the test and totally forgot to change it before I created the patch.


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66439
-----------------------------------------------------------


On Dec. 31, 2014, 1:46 p.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 1:46 p.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Josh Elser <jo...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66439
-----------------------------------------------------------


Neat stuff, Corey!


core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java
<https://reviews.apache.org/r/29502/#comment109983>

    Can't you pull this from the Scanner?



server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java
<https://reviews.apache.org/r/29502/#comment109984>

    It looks like TabletIteratorEnvironment is used for minor compactions. Isn't always setting `Authorizations.EMPTY` a little misleading? Is there something more representative of having all auths we could do here? Maybe extra documentation is enough? Could also throw UnsupportedOperationException or similar when the IteratorScope is something that isn't SCAN?



test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java
<https://reviews.apache.org/r/29502/#comment109986>

    Please create a user, assign it the auths you need, and then remove the user after the test.
    
    If this test is run against a standalone instance, it should try to leave the system in the same state the test started in.



test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java
<https://reviews.apache.org/r/29502/#comment109985>

    Close the batchwriter.


- Josh Elser


On Dec. 31, 2014, 4:05 a.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 4:05 a.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.


> On Jan. 5, 2015, 9:09 p.m., Christopher Tubbs wrote:
> > core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java, line 63
> > <https://reviews.apache.org/r/29502/diff/2/?file=804705#file804705line63>
> >
> >     Should this be Authorizations.EMPTY? Or should it have a default implementation on WrappingIterator which calls source.getAuthorizations()?
> 
> Christopher Tubbs wrote:
>     make that `getSource().getAuthorizations()`

Specific to this test I returned null because all the other getters (other than what was being explicitly tested) were returning null. Were you thinking WrappingIterator should also provide a getAuthorizations() method?


> On Jan. 5, 2015, 9:09 p.m., Christopher Tubbs wrote:
> > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java, line 46
> > <https://reviews.apache.org/r/29502/diff/2/?file=804711#file804711line46>
> >
> >     I wonder if there's a better way to provide environment options, like this and others, at specific scopes. Maybe use some dependency injection, with annotations, like Servlet @Context or JUnit @Rule: @ScanContext Authorizations auths; (throw error if type is not appropriate for context during injection).

This feature would be pretty neat. Were you thinking this would extend past just the IteratorEnvironment into other places? Any other fields you can think of that would benefit from this change other than Authorizations?


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66725
-----------------------------------------------------------


On Dec. 31, 2014, 3:40 p.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 3:40 p.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Christopher Tubbs <ct...@apache.org>.


> On Jan. 5, 2015, 4:09 p.m., Christopher Tubbs wrote:
> > core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java, line 63
> > <https://reviews.apache.org/r/29502/diff/2/?file=804705#file804705line63>
> >
> >     Should this be Authorizations.EMPTY? Or should it have a default implementation on WrappingIterator which calls source.getAuthorizations()?

make that `getSource().getAuthorizations()`


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66725
-----------------------------------------------------------


On Dec. 31, 2014, 10:40 a.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 10:40 a.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Christopher Tubbs <ct...@apache.org>.


> On Jan. 5, 2015, 4:09 p.m., Christopher Tubbs wrote:
> > core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java, line 63
> > <https://reviews.apache.org/r/29502/diff/2/?file=804705#file804705line63>
> >
> >     Should this be Authorizations.EMPTY? Or should it have a default implementation on WrappingIterator which calls source.getAuthorizations()?
> 
> Christopher Tubbs wrote:
>     make that `getSource().getAuthorizations()`
> 
> Corey Nolet wrote:
>     Specific to this test I returned null because all the other getters (other than what was being explicitly tested) were returning null. Were you thinking WrappingIterator should also provide a getAuthorizations() method?

Oh, nevermind. I misunderstood the change. This is fine.


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66725
-----------------------------------------------------------


On Jan. 6, 2015, 10:54 a.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 10:54 a.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Christopher Tubbs <ct...@apache.org>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66725
-----------------------------------------------------------



core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java
<https://reviews.apache.org/r/29502/#comment110326>

    Should this be Authorizations.EMPTY? Or should it have a default implementation on WrappingIterator which calls source.getAuthorizations()?



server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java
<https://reviews.apache.org/r/29502/#comment110327>

    I wonder if there's a better way to provide environment options, like this and others, at specific scopes. Maybe use some dependency injection, with annotations, like Servlet @Context or JUnit @Rule: @ScanContext Authorizations auths; (throw error if type is not appropriate for context during injection).


- Christopher Tubbs


On Dec. 31, 2014, 10:40 a.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 10:40 a.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by ke...@deenlo.com.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review67526
-----------------------------------------------------------

Ship it!


Ship It!

- kturner


On Jan. 9, 2015, 3:11 a.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2015, 3:11 a.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/
-----------------------------------------------------------

(Updated Jan. 9, 2015, 3:11 a.m.)


Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.


Bugs: ACCUMULO-3458
    https://issues.apache.org/jira/browse/ACCUMULO-3458


Repository: accumulo


Description
-------

ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.


Diffs (updated)
-----

  core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
  core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
  core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
  core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
  core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
  core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
  core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
  core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
  core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
  core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
  core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
  core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
  server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
  server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
  server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
  test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
  test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29502/diff/


Testing
-------

Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment


Thanks,

Corey Nolet

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by ke...@deenlo.com.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66908
-----------------------------------------------------------



test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java
<https://reviews.apache.org/r/29502/#comment110613>

    Thinking a bit more about the batchScanner, `runTest()` could call another parameterized test that accepts a scanner.  This would make it easy to test scanner and batch scanner.  A reason to do this is if the batch scanner sets up iterators differently and does not pass auths in env, the test might catch it.
    
        runTest(auths, shouldFail){
           runTest(scanner, auths, shouldFail);
           runTest(batchScanner, auths, shouldFail);
           batchScanner.close();
        }


- kturner


On Jan. 6, 2015, 3:54 p.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 3:54 p.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/
-----------------------------------------------------------

(Updated Jan. 6, 2015, 3:54 p.m.)


Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.


Changes
-------

Removing files which were formatted but not changed in any other way to augment the feature in the commit.


Bugs: ACCUMULO-3458
    https://issues.apache.org/jira/browse/ACCUMULO-3458


Repository: accumulo


Description
-------

ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.


Diffs (updated)
-----

  core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
  core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
  core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
  core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
  core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
  core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
  core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
  core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
  core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
  core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
  core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
  core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
  server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
  server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
  server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
  test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
  test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29502/diff/


Testing
-------

Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment


Thanks,

Corey Nolet

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/
-----------------------------------------------------------

(Updated Jan. 6, 2015, 3:44 p.m.)


Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.


Changes
-------

Fixed based on feedback from Christopher and Keith. Noticed some extra formatting removing whitespace in some places.


Bugs: ACCUMULO-3458
    https://issues.apache.org/jira/browse/ACCUMULO-3458


Repository: accumulo


Description
-------

ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.


Diffs (updated)
-----

  core/src/main/java/org/apache/accumulo/core/client/BatchDeleter.java 2bfc347 
  core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
  core/src/main/java/org/apache/accumulo/core/client/Scanner.java 112179e 
  core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
  core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerIterator.java 1e0ac99 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
  core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
  core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
  core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
  core/src/main/java/org/apache/accumulo/core/iterators/WrappingIterator.java 060fa76 
  core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
  core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
  core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
  core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
  core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
  core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
  core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
  server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
  server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
  server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
  test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
  test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29502/diff/


Testing
-------

Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment


Thanks,

Corey Nolet

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.


> On Jan. 1, 2015, 12:36 a.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java, line 78
> > <https://reviews.apache.org/r/29502/diff/2/?file=804700#file804700line78>
> >
> >     was putting @Override on same line as method decleration intentional?
> 
> Christopher Tubbs wrote:
>     Probably best to just format and organize imports for all the changed files. I noticed a lot of other formatting issues, too.
> 
> Corey Nolet wrote:
>     Not sure why intelli-j defaults to this behavior but it's fixed.
> 
> Christopher Tubbs wrote:
>     Import order is something that our formatting standards don't even address, I just noticed the change and thought it unusual.

This is something we worked out on Fluo early on and I believe the static changing from the top of the imports to the bottom was a result of that- though I'm surprised, unless Keith has multiple profiles for his import orders, why we wouldn't have noticed this sooner in his patches.

See https://github.com/fluo-io/fluo/wiki/Contributing#coding-guidelines


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66493
-----------------------------------------------------------


On Jan. 6, 2015, 3:54 p.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 3:54 p.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.


> On Jan. 1, 2015, 12:36 a.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java, line 78
> > <https://reviews.apache.org/r/29502/diff/2/?file=804700#file804700line78>
> >
> >     was putting @Override on same line as method decleration intentional?
> 
> Christopher Tubbs wrote:
>     Probably best to just format and organize imports for all the changed files. I noticed a lot of other formatting issues, too.
> 
> Corey Nolet wrote:
>     Not sure why intelli-j defaults to this behavior but it's fixed.
> 
> Christopher Tubbs wrote:
>     Import order is something that our formatting standards don't even address, I just noticed the change and thought it unusual.
> 
> Corey Nolet wrote:
>     This is something we worked out on Fluo early on and I believe the static changing from the top of the imports to the bottom was a result of that- though I'm surprised, unless Keith has multiple profiles for his import orders, why we wouldn't have noticed this sooner in his patches.
>     
>     See https://github.com/fluo-io/fluo/wiki/Contributing#coding-guidelines
> 
> kturner wrote:
>     > unless Keith has multiple profiles for his import orders
>     
>     I use two eclipse workspaces w/ different config, one for Fluo and one for Accumulo.  I try my best to avoid making changes unrelated to the task I am working.

Christopher, when I read over your comment early this morning, I read it as "Import order is something that our formatting standards don't even address,  [maybe it's time we address them.]" I referenced what we did on Fluo just to present the idea of putting those standards on the site, not to say any they need to be changed.

> I use two eclipse workspaces w/ different config, one for Fluo and one for Accumulo
    
It makes sense now. I was wondering why the deltas were only happening for me. I just assumed Mike and yourself were using the defaults that were already being used in Accumulo.


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66493
-----------------------------------------------------------


On Jan. 6, 2015, 3:54 p.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 3:54 p.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Christopher Tubbs <ct...@apache.org>.


> On Dec. 31, 2014, 7:36 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java, line 78
> > <https://reviews.apache.org/r/29502/diff/2/?file=804700#file804700line78>
> >
> >     was putting @Override on same line as method decleration intentional?

Probably best to just format and organize imports for all the changed files. I noticed a lot of other formatting issues, too.


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66493
-----------------------------------------------------------


On Dec. 31, 2014, 10:40 a.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 10:40 a.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Christopher Tubbs <ct...@apache.org>.


> On Dec. 31, 2014, 7:36 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java, line 78
> > <https://reviews.apache.org/r/29502/diff/2/?file=804700#file804700line78>
> >
> >     was putting @Override on same line as method decleration intentional?
> 
> Christopher Tubbs wrote:
>     Probably best to just format and organize imports for all the changed files. I noticed a lot of other formatting issues, too.
> 
> Corey Nolet wrote:
>     Not sure why intelli-j defaults to this behavior but it's fixed.

Import order is something that our formatting standards don't even address, I just noticed the change and thought it unusual.


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66493
-----------------------------------------------------------


On Jan. 6, 2015, 10:54 a.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 10:54 a.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by ke...@deenlo.com.


> On Jan. 1, 2015, 12:36 a.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java, line 78
> > <https://reviews.apache.org/r/29502/diff/2/?file=804700#file804700line78>
> >
> >     was putting @Override on same line as method decleration intentional?
> 
> Christopher Tubbs wrote:
>     Probably best to just format and organize imports for all the changed files. I noticed a lot of other formatting issues, too.
> 
> Corey Nolet wrote:
>     Not sure why intelli-j defaults to this behavior but it's fixed.
> 
> Christopher Tubbs wrote:
>     Import order is something that our formatting standards don't even address, I just noticed the change and thought it unusual.
> 
> Corey Nolet wrote:
>     This is something we worked out on Fluo early on and I believe the static changing from the top of the imports to the bottom was a result of that- though I'm surprised, unless Keith has multiple profiles for his import orders, why we wouldn't have noticed this sooner in his patches.
>     
>     See https://github.com/fluo-io/fluo/wiki/Contributing#coding-guidelines

> unless Keith has multiple profiles for his import orders

I use two eclipse workspaces w/ different config, one for Fluo and one for Accumulo.  I try my best to avoid making changes unrelated to the task I am working.


- kturner


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66493
-----------------------------------------------------------


On Jan. 6, 2015, 3:54 p.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 3:54 p.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Christopher Tubbs <ct...@apache.org>.


> On Dec. 31, 2014, 7:36 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java, line 78
> > <https://reviews.apache.org/r/29502/diff/2/?file=804700#file804700line78>
> >
> >     was putting @Override on same line as method decleration intentional?
> 
> Christopher Tubbs wrote:
>     Probably best to just format and organize imports for all the changed files. I noticed a lot of other formatting issues, too.
> 
> Corey Nolet wrote:
>     Not sure why intelli-j defaults to this behavior but it's fixed.
> 
> Christopher Tubbs wrote:
>     Import order is something that our formatting standards don't even address, I just noticed the change and thought it unusual.
> 
> Corey Nolet wrote:
>     This is something we worked out on Fluo early on and I believe the static changing from the top of the imports to the bottom was a result of that- though I'm surprised, unless Keith has multiple profiles for his import orders, why we wouldn't have noticed this sooner in his patches.
>     
>     See https://github.com/fluo-io/fluo/wiki/Contributing#coding-guidelines
> 
> kturner wrote:
>     > unless Keith has multiple profiles for his import orders
>     
>     I use two eclipse workspaces w/ different config, one for Fluo and one for Accumulo.  I try my best to avoid making changes unrelated to the task I am working.
> 
> Corey Nolet wrote:
>     Christopher, when I read over your comment early this morning, I read it as "Import order is something that our formatting standards don't even address,  [maybe it's time we address them.]" I referenced what we did on Fluo just to present the idea of putting those standards on the site, not to say any they need to be changed.
>     
>     > I use two eclipse workspaces w/ different config, one for Fluo and one for Accumulo
>         
>     It makes sense now. I was wondering why the deltas were only happening for me. I just assumed Mike and yourself were using the defaults that were already being used in Accumulo.

Corey, yes I guess I was hinting at an implied 'maybe it's time we address them'. But, I was also wanted to express that you shouldn't feel it necessary to make the change to comply with my preferences, since we haven't actually established a particular standard. In my initial comment, I didn't realize you weren't using Eclipse, but in my second comment, I was pointing out that even if you were using Eclipse with our formatter, it still wouldn't have (necessarily) addressed the import order if you had modified them from the defaults.


- Christopher


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66493
-----------------------------------------------------------


On Jan. 6, 2015, 10:54 a.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Jan. 6, 2015, 10:54 a.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ScannerImplTest.java be4d467 
>   core/src/test/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.


> On Jan. 1, 2015, 12:36 a.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java, line 78
> > <https://reviews.apache.org/r/29502/diff/2/?file=804700#file804700line78>
> >
> >     was putting @Override on same line as method decleration intentional?
> 
> Christopher Tubbs wrote:
>     Probably best to just format and organize imports for all the changed files. I noticed a lot of other formatting issues, too.

Not sure why intelli-j defaults to this behavior but it's fixed.


- Corey


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66493
-----------------------------------------------------------


On Dec. 31, 2014, 3:40 p.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 3:40 p.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by ke...@deenlo.com.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/#review66493
-----------------------------------------------------------



core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java
<https://reviews.apache.org/r/29502/#comment110073>

    Testing this method for scanner and batch scanner would be nice.



core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java
<https://reviews.apache.org/r/29502/#comment110070>

    was putting @Override on same line as method decleration intentional?



test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java
<https://reviews.apache.org/r/29502/#comment110071>

    could this be more strict, should the count be 1?



test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java
<https://reviews.apache.org/r/29502/#comment110072>

    Two more test would be nice.
    
     * Test the case where empty set of Auths is set on scanner, and verify AuthsIterator.FAIL is returned.
     * Test case where set Auths("B") is set on scanner, and verify AuthsIterator.FAIL is returned.


The TransformingIterator could possible be modified to leverage this change.  That could be done as a follow on issue.

- kturner


On Dec. 31, 2014, 3:40 p.m., Corey Nolet wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29502/
> -----------------------------------------------------------
> 
> (Updated Dec. 31, 2014, 3:40 p.m.)
> 
> 
> Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.
> 
> 
> Bugs: ACCUMULO-3458
>     https://issues.apache.org/jira/browse/ACCUMULO-3458
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.
> 
> 
> Diffs
> -----
> 
>   core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
>   core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
>   core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
>   core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
>   core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
>   core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
>   core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
>   core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
>   server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
>   server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
>   test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29502/diff/
> 
> 
> Testing
> -------
> 
> Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment
> 
> 
> Thanks,
> 
> Corey Nolet
> 
>

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/
-----------------------------------------------------------

(Updated Dec. 31, 2014, 10:40 a.m.)


Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.


Bugs: ACCUMULO-3458
    https://issues.apache.org/jira/browse/ACCUMULO-3458


Repository: accumulo


Description
-------

ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.


Diffs
-----

  core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
  core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
  core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
  core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
  core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
  core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
  core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
  core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
  core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
  core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
  core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
  server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
  server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
  server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
  test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
  test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29502/diff/


Testing
-------

Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment


Thanks,

Corey Nolet

Re: Review Request 29502: ACCUMULO-3458 Adding scan authorizations to IteratorEnvironment

Posted by Corey Nolet <cj...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29502/
-----------------------------------------------------------

(Updated Dec. 31, 2014, 1:46 p.m.)


Review request for accumulo, Christopher Tubbs, Eric Newton, Josh Elser, and kturner.


Repository: accumulo


Description
-------

ACCUMULO-3458 Propagating scan-time authorizations through the IteratorEnvironment so that scan-time iterators can use them.


Diffs (updated)
-----

  core/src/main/java/org/apache/accumulo/core/client/ClientSideIteratorScanner.java 4903656 
  core/src/main/java/org/apache/accumulo/core/client/ScannerBase.java 335b63a 
  core/src/main/java/org/apache/accumulo/core/client/impl/OfflineScanner.java 2552682 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 666a8af 
  core/src/main/java/org/apache/accumulo/core/client/impl/ScannerOptions.java 9726266 
  core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReader.java 2a79f05 
  core/src/main/java/org/apache/accumulo/core/client/mock/MockScannerBase.java 72cb863 
  core/src/main/java/org/apache/accumulo/core/iterators/IteratorEnvironment.java 9e20cb1 
  core/src/main/java/org/apache/accumulo/core/iterators/system/VisibilityFilter.java 15c33fa 
  core/src/test/java/org/apache/accumulo/core/iterators/DefaultIteratorEnvironment.java 94da7b5 
  core/src/test/java/org/apache/accumulo/core/iterators/FirstEntryInRowIteratorTest.java fa46360 
  core/src/test/java/org/apache/accumulo/core/iterators/user/RowDeletingIteratorTest.java 4521e55 
  core/src/test/java/org/apache/accumulo/core/iterators/user/TransformingIteratorTest.java 4cebab7 
  server/base/src/test/java/org/apache/accumulo/server/iterators/MetadataBulkLoadFilterTest.java 4a45e99 
  server/base/src/test/java/org/apache/accumulo/server/replication/StatusCombinerTest.java a9801b0 
  server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/NullScanner.java bf35557 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletIteratorEnvironment.java d1fece5 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Compactor.java 869cc33 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/ScanDataSource.java fe4b16b 
  test/src/main/java/org/apache/accumulo/test/functional/AuthsIterator.java PRE-CREATION 
  test/src/test/java/org/apache/accumulo/test/ScanIteratorIT.java PRE-CREATION 

Diff: https://reviews.apache.org/r/29502/diff/


Testing
-------

Wrote an integration test to verify that ScanDataSource is actually setting the authorizations on the IteratorEnvironment


Thanks,

Corey Nolet