date:20150107



 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 29386: ACCUMULO-2815 Client authentication via Kerberos

On Jan. 5, 2015, 8:46 p.m., Christopher Tubbs wrote:
server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java,
lines 68-75
https://reviews.apache.org/r/29386/diff/9/?file=804864#file804864line68

This makes the assumption that this method is only called on the
current user. That's not true. It also supports the public API
SecurityOperations.authenticateUser()

Josh Elser wrote:
Oh, hrm. That might be problematic? I have to look at where the token
comes from. We need to differentiate between the token for the user asking,
and the token for the user being asked about. The API ensures that we can at
least have both of those tokens, which gives us what we need.

I think we can proxy the second token on top of the current token to
validate (the user being asked about on top of the user which is doing the
asking). If that succeeds, the user being asked about should be able to
access the system on their own and everything else should be transparent.

I need to write some tests to ensure multi-user permissions operate
expectedly (I'm working on MAC being able to use a KDC now).

So, the reason this struck me as odd is that I don't know how this is actually
possible :)

I can't ask if another Kerberos user's credentials are valid because I
shouldn't have them (and I don't *think* it's possible to have multiple active
credentials passed over the wire). I think I just need to make a check that the
caller's credentials are the same as the credentials we're being asked to
check. Or, I'm not sure if this is impossible or I just don't know how to do it.

- Josh

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

On Jan. 7, 2015, 9:37 p.m., Josh Elser wrote:

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

(Updated Jan. 7, 2015, 9:37 p.m.)

Review request for accumulo.

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

Repository: accumulo

Description
---

ACCUMULO-2815 Initial support for Kerberos client authentication.

Leverage SASL transport provided by Thrift which can speak GSSAPI, which
Kerberos implements. Introduced...

* An Accumulo KerberosToken which is an AuthenticationToken to validate users.
* Custom thrift processor and invocation handler to ensure server RPCs have a
valid KRB identity and Accumulo authentication.
* A KerberosAuthenticator which extends ZKAuthenticator to support Kerberos
identities seamlessly.
* New ClientConf variables to use SASL transport and pass Kerberos server
principal
* Updated ClientOpts and Shell opts to transparently use a KerberosToken when
SASL is enabled (no extra client work).

I believe this is the bare minimum for Kerberos support. They are also
grossly lacking in unit and integration tests. I believe that I might have
somehow broken the client address string in the server (I saw log messages
with client: null, but I'm not sure if it's due to these changes or not). A
necessary limitation in the Thrift server used is that, like the SSL
transport, the SASL transport cannot presently be used with the
TFramedTransport, which means none of the [half]async thrift servers will
function with this -- we're stuck with the TThreadPoolServer.

Performed some contrived benchmarks on my laptop (while still using it
myself) to get at big-picture view of the performance impact against normal
operation and Kerberos alone. Each run was the duration to ingest 100M
records using continuous-ingest, timed with `time`, using 'real'.

THsHaServer (our default), 6 runs:

Avg: 10m7.273s (607.273s)
Min: 9m43.395s
Max: 10m52.715s

TThreadPoolServer (no SASL), 5 runs:

Avg: 11m16.254s (676.254s)
Min: 10m30.987s
Max: 12m24.192s

TThreadPoolServer+SASL/GSSAPI (these changes), 6 runs:

Avg: 13m17.187s (797.187s)
Min: 10m52.997s
Max: 16m0.975s

The general takeway is that there's about 15% performance degredation in its
initial state which is in the realm of what I expected (~10%).

Diffs
-

core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java f6ea934
core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java
6fe61a5
core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java
e75bec6
core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java
f481cc3
core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java
a9ad8a1

core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java
6dc846f

Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos

2015-01-07 Thread Christopher Tubbs

On Jan. 5, 2015, 3:46 p.m., Christopher Tubbs wrote:
server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java,
lines 68-75
https://reviews.apache.org/r/29386/diff/9/?file=804864#file804864line68

This makes the assumption that this method is only called on the
current user. That's not true. It also supports the public API
SecurityOperations.authenticateUser()

I need to write some tests to ensure multi-user permissions operate
expectedly (I'm working on MAC being able to use a KDC now).

Josh Elser wrote:
So, the reason this struck me as odd is that I don't know how this is
actually possible :)

I can't ask if another Kerberos user's credentials are valid because I
shouldn't have them (and I don't *think* it's possible to have multiple
active credentials passed over the wire). I think I just need to make a check
that the caller's credentials are the same as the credentials we're being
asked to check. Or, I'm not sure if this is impossible or I just don't know
how to do it.

Well, it could be possible if the token was not simply a hint to use the
Kerberos login elsewhere, but instead actually contained parameters used to log
in with Kerberos. (See my comment under the conversation related to ugi in the
public API from Keith, above). But, we'd have to jump through some hoops with
some trick to defer using those creds to actually log in, until it gets to the
server side in the case it is being asked about, but do it on the client side
for the one asking. I don't think that's worth it.

As is, I agree, it's not possible. Instead, this method should be bypassed, and
whichever method calls it should make the check, based on whether it is the
current user (which we know to be authenticated) or if it's another user (which
we cannot authenticate). This method can't do that, because it knows nothing of
the current user. So, it should just be an unsupported operation.

- Christopher

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

On Jan. 7, 2015, 4:37 p.m., Josh Elser wrote:

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

(Updated Jan. 7, 2015, 4:37 p.m.)

Review request for accumulo.

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

Repository: accumulo

Description
---

ACCUMULO-2815 Initial support for Kerberos client authentication.

Leverage SASL transport provided by Thrift which can speak GSSAPI, which
Kerberos implements. Introduced...

THsHaServer (our default), 6 runs:

Avg: 10m7.273s (607.273s)

[VOTE][LAZY] Format all supported branches

To make it easier to apply some minimal checkstyle rules for ACCUMULO-3451,
I'm announcing my intentions to do a full, one-time, auto-format and
organize imports on all our supported branches (1.5, 1.6, and master) to
bring us up to some degree of compliance with our agreed-upon formatting
standards.

Benefits:
To have additional checks, in particular against javadoc problems and other
common trivial warnings in the build.
To ensure less divergence from our agreed-upon formatting standards.
Formatting first makes it much less tedious and easier on me to add these
checks to the build.

Issues I've considered:
I will deal with all the merge conflicts.
I will ignore generated thrift code.
Conflicts with new code in people's branches should be minimal (and easily
resolved by formatting according to our standards).
Regarding concerns about history tracking, in general, each format change
is small, but they are numerous. So, the impact on tracking history should
be very minimal (you'll see things like a brace moved to the same line as
the else statement it is associated with... stuff that won't generally
affect your ability to debug).
I'll also do a format only commit, separately from any substantive
changes regarding the rule changes, so the mass formatting change will
happen in one place, and it will also be easy to revert, if absolutely
necessary.

I'll give this 24 hours (it can be reverted if somebody objects after that).

--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

Re: Review Request 29230: ACCUMULO-3439 Add RegexGroupBalancer


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

(Updated Jan. 7, 2015, 7:52 p.m.)


Review request for accumulo.


Changes
---

Reformatted files.  Also turned on eclipse save actions to make stuff final, 
add @Override, and remove trailing spaces.


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


Repository: accumulo


Description
---

The patch contains a new balancer, test, and documentation.


Diffs (updated)
-

  docs/src/main/resources/examples/README.rgbalancer PRE-CREATION 
  
server/base/src/main/java/org/apache/accumulo/server/master/balancer/GroupBalancer.java
 PRE-CREATION 
  
server/base/src/main/java/org/apache/accumulo/server/master/balancer/RegexGroupBalancer.java
 PRE-CREATION 
  
server/base/src/main/java/org/apache/accumulo/server/master/balancer/TableLoadBalancer.java
 5eae890 
  
server/base/src/test/java/org/apache/accumulo/server/master/balancer/GroupBalancerTest.java
 PRE-CREATION 
  
test/src/test/java/org/apache/accumulo/test/functional/RegexGroupBalanceIT.java 
PRE-CREATION 

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


Testing
---

The new unit test and intergeration test added in the patch run.  


Thanks,

kturner

Re: [VOTE][LAZY] Format all supported branches

2015-01-07 Thread John Vines

+1

On Wed, Jan 7, 2015 at 3:12 PM, Christopher ctubb...@apache.org wrote:

 To make it easier to apply some minimal checkstyle rules for ACCUMULO-3451,
 I'm announcing my intentions to do a full, one-time, auto-format and
 organize imports on all our supported branches (1.5, 1.6, and master) to
 bring us up to some degree of compliance with our agreed-upon formatting
 standards.

 Benefits:
 To have additional checks, in particular against javadoc problems and other
 common trivial warnings in the build.
 To ensure less divergence from our agreed-upon formatting standards.
 Formatting first makes it much less tedious and easier on me to add these
 checks to the build.

 Issues I've considered:
 I will deal with all the merge conflicts.
 I will ignore generated thrift code.
 Conflicts with new code in people's branches should be minimal (and easily
 resolved by formatting according to our standards).
 Regarding concerns about history tracking, in general, each format change
 is small, but they are numerous. So, the impact on tracking history should
 be very minimal (you'll see things like a brace moved to the same line as
 the else statement it is associated with... stuff that won't generally
 affect your ability to debug).
 I'll also do a format only commit, separately from any substantive
 changes regarding the rule changes, so the mass formatting change will
 happen in one place, and it will also be easy to revert, if absolutely
 necessary.

 I'll give this 24 hours (it can be reverted if somebody objects after
 that).

 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

So the steps I was performing were:

1) Eclipse format all java files
2) Eclipse organize all imports
3) Command-line sed to strip trailing whitespace (which gets those pesky
indented empty javadoc lines)
find . -name '*.java' -print0 | xargs -0 sed -i 's/ *$//'
4) Replace tab indents with spaces (these really surprised me, and I just
manually fixed them)
vim $(grep -R $'\t' . | cut -f 1 -d : | grep '\.java$' | sort | uniq)
5) Rebuild with -Pthrift, to ensure I don't touch generated files
mvn package -DskipTests -Pthrift

#3 and #4 can't be easily done in Eclipse, but they are trivial sed/grep
command lines. I wasn't planning on producing any script. I'd rather just
point people to the sed/grep man pages, and the right menu options in
Eclipse if they need to reproduce.

After these steps, merged with -sours and repeated in each branch, the next
two commits would simply be: add checkstyle rules to the pom, and modify
remaining problems/errors/etc. to make rules pass. That part is easy, since
I have a reasonable set of rules to work with.

For Brian's concerns about -sours, I always check that the commit prior to
the commits I intend to merge with -sours are already merged normally and
conflicts resolved, so that the only commit merged with -sours is the one I
intend (I actually kind of wish git had a built-in option/warning for that
check... but I always do it anyway). -srecursive -Xours could still miss
things, because it could improperly resolve some conflicts. In my case,
they'd be equivalent, though.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 3:47 PM, Mike Drob mad...@cloudera.com wrote:

 Also, if you're using Eclipse to do the auto-format, please check for
 trailing white-space on otherwise empty javadoc lines.

 If you automate this in some fashion outside of Eclipse (because other
 people may prefer other editors), this would be a useful script to add to a
 top-level dev-tools folder.

 On Wed, Jan 7, 2015 at 2:36 PM, David Medinets david.medin...@gmail.com
 wrote:

  +1
 
  Are you automating the process so that you can re-apply the same steps
  in one year?
 
  On Wed, Jan 7, 2015 at 3:31 PM, Christopher ctubb...@apache.org wrote:
   Can do. It's a bit more work for me, because I have to repeat the same
   actions over and over again, but if it helps history look a little
  cleaner,
   i can do it, and just stick to -sours and repeat for the next branch..
  
  
   --
   Christopher L Tubbs II
   http://gravatar.com/ctubbsii
  
   On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com wrote:
  
   Please do not do formatting during merge conflict resolution, and make
   those be separate commits.
  
   On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser josh.el...@gmail.com
  wrote:
  
ack'ed
   
   
John Vines wrote:
   
+1
   
On Wed, Jan 7, 2015 at 3:12 PM, Christopherctubb...@apache.org
   wrote:
   
 To make it easier to apply some minimal checkstyle rules for
ACCUMULO-3451,
I'm announcing my intentions to do a full, one-time, auto-format
 and
organize imports on all our supported branches (1.5, 1.6, and
  master)
   to
bring us up to some degree of compliance with our agreed-upon
   formatting
standards.
   
Benefits:
To have additional checks, in particular against javadoc problems
  and
other
common trivial warnings in the build.
To ensure less divergence from our agreed-upon formatting
 standards.
Formatting first makes it much less tedious and easier on me to
 add
   these
checks to the build.
   
Issues I've considered:
I will deal with all the merge conflicts.
I will ignore generated thrift code.
Conflicts with new code in people's branches should be minimal
 (and
easily
resolved by formatting according to our standards).
Regarding concerns about history tracking, in general, each format
   change
is small, but they are numerous. So, the impact on tracking
 history
should
be very minimal (you'll see things like a brace moved to the same
  line
   as
the else statement it is associated with... stuff that won't
  generally
affect your ability to debug).
I'll also do a format only commit, separately from any
 substantive
changes regarding the rule changes, so the mass formatting change
  will
happen in one place, and it will also be easy to revert, if
  absolutely
necessary.
   
I'll give this 24 hours (it can be reverted if somebody objects
  after
that).
   
--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

2015-01-07 Thread Mike Drob

Will the checkstyle rules fail the build or just print warnings?

On Wed, Jan 7, 2015 at 3:11 PM, Christopher ctubb...@apache.org wrote:

 In the long run, the rules will (hopefully) save me work following up
 fixing bad javadocs and trivial warnings.


 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

 On Wed, Jan 7, 2015 at 4:03 PM, Eric Newton eric.new...@gmail.com wrote:

  +0
 
  I don't think it's worth the disruption, but I don't mind if you're going
  to do all the work.
 
 
  On Wed, Jan 7, 2015 at 3:47 PM, Mike Drob mad...@cloudera.com wrote:
 
   Also, if you're using Eclipse to do the auto-format, please check for
   trailing white-space on otherwise empty javadoc lines.
  
   If you automate this in some fashion outside of Eclipse (because other
   people may prefer other editors), this would be a useful script to add
  to a
   top-level dev-tools folder.
  
   On Wed, Jan 7, 2015 at 2:36 PM, David Medinets 
 david.medin...@gmail.com
  
   wrote:
  
+1
   
Are you automating the process so that you can re-apply the same
 steps
in one year?
   
On Wed, Jan 7, 2015 at 3:31 PM, Christopher ctubb...@apache.org
  wrote:
 Can do. It's a bit more work for me, because I have to repeat the
  same
 actions over and over again, but if it helps history look a little
cleaner,
 i can do it, and just stick to -sours and repeat for the next
  branch..


 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

 On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com
  wrote:

 Please do not do formatting during merge conflict resolution, and
  make
 those be separate commits.

 On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser josh.el...@gmail.com
wrote:

  ack'ed
 
 
  John Vines wrote:
 
  +1
 
  On Wed, Jan 7, 2015 at 3:12 PM, Christopher
 ctubb...@apache.org
 wrote:
 
   To make it easier to apply some minimal checkstyle rules for
  ACCUMULO-3451,
  I'm announcing my intentions to do a full, one-time,
 auto-format
   and
  organize imports on all our supported branches (1.5, 1.6, and
master)
 to
  bring us up to some degree of compliance with our agreed-upon
 formatting
  standards.
 
  Benefits:
  To have additional checks, in particular against javadoc
  problems
and
  other
  common trivial warnings in the build.
  To ensure less divergence from our agreed-upon formatting
   standards.
  Formatting first makes it much less tedious and easier on me
 to
   add
 these
  checks to the build.
 
  Issues I've considered:
  I will deal with all the merge conflicts.
  I will ignore generated thrift code.
  Conflicts with new code in people's branches should be minimal
   (and
  easily
  resolved by formatting according to our standards).
  Regarding concerns about history tracking, in general, each
  format
 change
  is small, but they are numerous. So, the impact on tracking
   history
  should
  be very minimal (you'll see things like a brace moved to the
  same
line
 as
  the else statement it is associated with... stuff that won't
generally
  affect your ability to debug).
  I'll also do a format only commit, separately from any
   substantive
  changes regarding the rule changes, so the mass formatting
  change
will
  happen in one place, and it will also be easy to revert, if
absolutely
  necessary.
 
  I'll give this 24 hours (it can be reverted if somebody
 objects
after
  that).
 
  --
  Christopher L Tubbs II
  http://gravatar.com/ctubbsii

Re: Review Request 29230: ACCUMULO-3439 Add RegexGroupBalancer



 On Jan. 6, 2015, 9:41 p.m., Eric Newton wrote:
  Nit: whitespace
  Nit: single-line statements w/out braces

I also turned on eclipse save action to add braces for 2nd patch


- kturner


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


On Jan. 7, 2015, 7:52 p.m., kturner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29230/
 ---
 
 (Updated Jan. 7, 2015, 7:52 p.m.)
 
 
 Review request for accumulo.
 
 
 Bugs: ACCUMULO-3439
 https://issues.apache.org/jira/browse/ACCUMULO-3439
 
 
 Repository: accumulo
 
 
 Description
 ---
 
 The patch contains a new balancer, test, and documentation.
 
 
 Diffs
 -
 
   docs/src/main/resources/examples/README.rgbalancer PRE-CREATION 
   
 server/base/src/main/java/org/apache/accumulo/server/master/balancer/GroupBalancer.java
  PRE-CREATION 
   
 server/base/src/main/java/org/apache/accumulo/server/master/balancer/RegexGroupBalancer.java
  PRE-CREATION 
   
 server/base/src/main/java/org/apache/accumulo/server/master/balancer/TableLoadBalancer.java
  5eae890 
   
 server/base/src/test/java/org/apache/accumulo/server/master/balancer/GroupBalancerTest.java
  PRE-CREATION 
   
 test/src/test/java/org/apache/accumulo/test/functional/RegexGroupBalanceIT.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29230/diff/
 
 
 Testing
 ---
 
 The new unit test and intergeration test added in the patch run.  
 
 
 Thanks,
 
 kturner

Re: [VOTE][LAZY] Format all supported branches


ack'ed

John Vines wrote:

+1

On Wed, Jan 7, 2015 at 3:12 PM, Christopherctubb...@apache.org  wrote:


To make it easier to apply some minimal checkstyle rules for ACCUMULO-3451,
I'm announcing my intentions to do a full, one-time, auto-format and
organize imports on all our supported branches (1.5, 1.6, and master) to
bring us up to some degree of compliance with our agreed-upon formatting
standards.

Benefits:
To have additional checks, in particular against javadoc problems and other
common trivial warnings in the build.
To ensure less divergence from our agreed-upon formatting standards.
Formatting first makes it much less tedious and easier on me to add these
checks to the build.

Issues I've considered:
I will deal with all the merge conflicts.
I will ignore generated thrift code.
Conflicts with new code in people's branches should be minimal (and easily
resolved by formatting according to our standards).
Regarding concerns about history tracking, in general, each format change
is small, but they are numerous. So, the impact on tracking history should
be very minimal (you'll see things like a brace moved to the same line as
the else statement it is associated with... stuff that won't generally
affect your ability to debug).
I'll also do a format only commit, separately from any substantive
changes regarding the rule changes, so the mass formatting change will
happen in one place, and it will also be easy to revert, if absolutely
necessary.

I'll give this 24 hours (it can be reverted if somebody objects after
that).

--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

2015-01-07 Thread Mike Drob

Please do not do formatting during merge conflict resolution, and make
those be separate commits.

On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser josh.el...@gmail.com wrote:

 ack'ed


 John Vines wrote:

 +1

 On Wed, Jan 7, 2015 at 3:12 PM, Christopherctubb...@apache.org  wrote:

  To make it easier to apply some minimal checkstyle rules for
 ACCUMULO-3451,
 I'm announcing my intentions to do a full, one-time, auto-format and
 organize imports on all our supported branches (1.5, 1.6, and master) to
 bring us up to some degree of compliance with our agreed-upon formatting
 standards.

 Benefits:
 To have additional checks, in particular against javadoc problems and
 other
 common trivial warnings in the build.
 To ensure less divergence from our agreed-upon formatting standards.
 Formatting first makes it much less tedious and easier on me to add these
 checks to the build.

 Issues I've considered:
 I will deal with all the merge conflicts.
 I will ignore generated thrift code.
 Conflicts with new code in people's branches should be minimal (and
 easily
 resolved by formatting according to our standards).
 Regarding concerns about history tracking, in general, each format change
 is small, but they are numerous. So, the impact on tracking history
 should
 be very minimal (you'll see things like a brace moved to the same line as
 the else statement it is associated with... stuff that won't generally
 affect your ability to debug).
 I'll also do a format only commit, separately from any substantive
 changes regarding the rule changes, so the mass formatting change will
 happen in one place, and it will also be easy to revert, if absolutely
 necessary.

 I'll give this 24 hours (it can be reverted if somebody objects after
 that).

 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

Can do. It's a bit more work for me, because I have to repeat the same
actions over and over again, but if it helps history look a little cleaner,
i can do it, and just stick to -sours and repeat for the next branch..


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com wrote:

 Please do not do formatting during merge conflict resolution, and make
 those be separate commits.

 On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser josh.el...@gmail.com wrote:

  ack'ed
 
 
  John Vines wrote:
 
  +1
 
  On Wed, Jan 7, 2015 at 3:12 PM, Christopherctubb...@apache.org
 wrote:
 
   To make it easier to apply some minimal checkstyle rules for
  ACCUMULO-3451,
  I'm announcing my intentions to do a full, one-time, auto-format and
  organize imports on all our supported branches (1.5, 1.6, and master)
 to
  bring us up to some degree of compliance with our agreed-upon
 formatting
  standards.
 
  Benefits:
  To have additional checks, in particular against javadoc problems and
  other
  common trivial warnings in the build.
  To ensure less divergence from our agreed-upon formatting standards.
  Formatting first makes it much less tedious and easier on me to add
 these
  checks to the build.
 
  Issues I've considered:
  I will deal with all the merge conflicts.
  I will ignore generated thrift code.
  Conflicts with new code in people's branches should be minimal (and
  easily
  resolved by formatting according to our standards).
  Regarding concerns about history tracking, in general, each format
 change
  is small, but they are numerous. So, the impact on tracking history
  should
  be very minimal (you'll see things like a brace moved to the same line
 as
  the else statement it is associated with... stuff that won't generally
  affect your ability to debug).
  I'll also do a format only commit, separately from any substantive
  changes regarding the rule changes, so the mass formatting change will
  happen in one place, and it will also be easy to revert, if absolutely
  necessary.
 
  I'll give this 24 hours (it can be reverted if somebody objects after
  that).
 
  --
  Christopher L Tubbs II
  http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

2015-01-07 Thread David Medinets

+1

Are you automating the process so that you can re-apply the same steps
in one year?

On Wed, Jan 7, 2015 at 3:31 PM, Christopher ctubb...@apache.org wrote:
 Can do. It's a bit more work for me, because I have to repeat the same
 actions over and over again, but if it helps history look a little cleaner,
 i can do it, and just stick to -sours and repeat for the next branch..


 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

 On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com wrote:

 Please do not do formatting during merge conflict resolution, and make
 those be separate commits.

 On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser josh.el...@gmail.com wrote:

  ack'ed
 
 
  John Vines wrote:
 
  +1
 
  On Wed, Jan 7, 2015 at 3:12 PM, Christopherctubb...@apache.org
 wrote:
 
   To make it easier to apply some minimal checkstyle rules for
  ACCUMULO-3451,
  I'm announcing my intentions to do a full, one-time, auto-format and
  organize imports on all our supported branches (1.5, 1.6, and master)
 to
  bring us up to some degree of compliance with our agreed-upon
 formatting
  standards.
 
  Benefits:
  To have additional checks, in particular against javadoc problems and
  other
  common trivial warnings in the build.
  To ensure less divergence from our agreed-upon formatting standards.
  Formatting first makes it much less tedious and easier on me to add
 these
  checks to the build.
 
  Issues I've considered:
  I will deal with all the merge conflicts.
  I will ignore generated thrift code.
  Conflicts with new code in people's branches should be minimal (and
  easily
  resolved by formatting according to our standards).
  Regarding concerns about history tracking, in general, each format
 change
  is small, but they are numerous. So, the impact on tracking history
  should
  be very minimal (you'll see things like a brace moved to the same line
 as
  the else statement it is associated with... stuff that won't generally
  affect your ability to debug).
  I'll also do a format only commit, separately from any substantive
  changes regarding the rule changes, so the mass formatting change will
  happen in one place, and it will also be easy to revert, if absolutely
  necessary.
 
  I'll give this 24 hours (it can be reverted if somebody objects after
  that).
 
  --
  Christopher L Tubbs II
  http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

2015-01-07 Thread Mike Drob

Also, if you're using Eclipse to do the auto-format, please check for
trailing white-space on otherwise empty javadoc lines.

If you automate this in some fashion outside of Eclipse (because other
people may prefer other editors), this would be a useful script to add to a
top-level dev-tools folder.

On Wed, Jan 7, 2015 at 2:36 PM, David Medinets david.medin...@gmail.com
wrote:

 +1

 Are you automating the process so that you can re-apply the same steps
 in one year?

 On Wed, Jan 7, 2015 at 3:31 PM, Christopher ctubb...@apache.org wrote:
  Can do. It's a bit more work for me, because I have to repeat the same
  actions over and over again, but if it helps history look a little
 cleaner,
  i can do it, and just stick to -sours and repeat for the next branch..
 
 
  --
  Christopher L Tubbs II
  http://gravatar.com/ctubbsii
 
  On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com wrote:
 
  Please do not do formatting during merge conflict resolution, and make
  those be separate commits.
 
  On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser josh.el...@gmail.com
 wrote:
 
   ack'ed
  
  
   John Vines wrote:
  
   +1
  
   On Wed, Jan 7, 2015 at 3:12 PM, Christopherctubb...@apache.org
  wrote:
  
To make it easier to apply some minimal checkstyle rules for
   ACCUMULO-3451,
   I'm announcing my intentions to do a full, one-time, auto-format and
   organize imports on all our supported branches (1.5, 1.6, and
 master)
  to
   bring us up to some degree of compliance with our agreed-upon
  formatting
   standards.
  
   Benefits:
   To have additional checks, in particular against javadoc problems
 and
   other
   common trivial warnings in the build.
   To ensure less divergence from our agreed-upon formatting standards.
   Formatting first makes it much less tedious and easier on me to add
  these
   checks to the build.
  
   Issues I've considered:
   I will deal with all the merge conflicts.
   I will ignore generated thrift code.
   Conflicts with new code in people's branches should be minimal (and
   easily
   resolved by formatting according to our standards).
   Regarding concerns about history tracking, in general, each format
  change
   is small, but they are numerous. So, the impact on tracking history
   should
   be very minimal (you'll see things like a brace moved to the same
 line
  as
   the else statement it is associated with... stuff that won't
 generally
   affect your ability to debug).
   I'll also do a format only commit, separately from any substantive
   changes regarding the rule changes, so the mass formatting change
 will
   happen in one place, and it will also be easy to revert, if
 absolutely
   necessary.
  
   I'll give this 24 hours (it can be reverted if somebody objects
 after
   that).
  
   --
   Christopher L Tubbs II
   http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

2015-01-07 Thread Eric Newton

+0

I don't think it's worth the disruption, but I don't mind if you're going
to do all the work.


On Wed, Jan 7, 2015 at 3:47 PM, Mike Drob mad...@cloudera.com wrote:

 Also, if you're using Eclipse to do the auto-format, please check for
 trailing white-space on otherwise empty javadoc lines.

 If you automate this in some fashion outside of Eclipse (because other
 people may prefer other editors), this would be a useful script to add to a
 top-level dev-tools folder.

 On Wed, Jan 7, 2015 at 2:36 PM, David Medinets david.medin...@gmail.com
 wrote:

  +1
 
  Are you automating the process so that you can re-apply the same steps
  in one year?
 
  On Wed, Jan 7, 2015 at 3:31 PM, Christopher ctubb...@apache.org wrote:
   Can do. It's a bit more work for me, because I have to repeat the same
   actions over and over again, but if it helps history look a little
  cleaner,
   i can do it, and just stick to -sours and repeat for the next branch..
  
  
   --
   Christopher L Tubbs II
   http://gravatar.com/ctubbsii
  
   On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com wrote:
  
   Please do not do formatting during merge conflict resolution, and make
   those be separate commits.
  
   On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser josh.el...@gmail.com
  wrote:
  
ack'ed
   
   
John Vines wrote:
   
+1
   
On Wed, Jan 7, 2015 at 3:12 PM, Christopherctubb...@apache.org
   wrote:
   
 To make it easier to apply some minimal checkstyle rules for
ACCUMULO-3451,
I'm announcing my intentions to do a full, one-time, auto-format
 and
organize imports on all our supported branches (1.5, 1.6, and
  master)
   to
bring us up to some degree of compliance with our agreed-upon
   formatting
standards.
   
Benefits:
To have additional checks, in particular against javadoc problems
  and
other
common trivial warnings in the build.
To ensure less divergence from our agreed-upon formatting
 standards.
Formatting first makes it much less tedious and easier on me to
 add
   these
checks to the build.
   
Issues I've considered:
I will deal with all the merge conflicts.
I will ignore generated thrift code.
Conflicts with new code in people's branches should be minimal
 (and
easily
resolved by formatting according to our standards).
Regarding concerns about history tracking, in general, each format
   change
is small, but they are numerous. So, the impact on tracking
 history
should
be very minimal (you'll see things like a brace moved to the same
  line
   as
the else statement it is associated with... stuff that won't
  generally
affect your ability to debug).
I'll also do a format only commit, separately from any
 substantive
changes regarding the rule changes, so the mass formatting change
  will
happen in one place, and it will also be easy to revert, if
  absolutely
necessary.
   
I'll give this 24 hours (it can be reverted if somebody objects
  after
that).
   
--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

Re: Review Request 29230: ACCUMULO-3439 Add RegexGroupBalancer



 On Jan. 6, 2015, 9:41 p.m., Eric Newton wrote:
  server/base/src/main/java/org/apache/accumulo/server/master/balancer/GroupBalancer.java,
   line 674
  https://reviews.apache.org/r/29230/diff/1/?file=796995#file796995line674
 
  Could this class be made static by passing in the table ID?

inner class also uses `context`.


- kturner


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


On Jan. 7, 2015, 7:52 p.m., kturner wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29230/
 ---
 
 (Updated Jan. 7, 2015, 7:52 p.m.)
 
 
 Review request for accumulo.
 
 
 Bugs: ACCUMULO-3439
 https://issues.apache.org/jira/browse/ACCUMULO-3439
 
 
 Repository: accumulo
 
 
 Description
 ---
 
 The patch contains a new balancer, test, and documentation.
 
 
 Diffs
 -
 
   docs/src/main/resources/examples/README.rgbalancer PRE-CREATION 
   
 server/base/src/main/java/org/apache/accumulo/server/master/balancer/GroupBalancer.java
  PRE-CREATION 
   
 server/base/src/main/java/org/apache/accumulo/server/master/balancer/RegexGroupBalancer.java
  PRE-CREATION 
   
 server/base/src/main/java/org/apache/accumulo/server/master/balancer/TableLoadBalancer.java
  5eae890 
   
 server/base/src/test/java/org/apache/accumulo/server/master/balancer/GroupBalancerTest.java
  PRE-CREATION 
   
 test/src/test/java/org/apache/accumulo/test/functional/RegexGroupBalanceIT.java
  PRE-CREATION 
 
 Diff: https://reviews.apache.org/r/29230/diff/
 
 
 Testing
 ---
 
 The new unit test and intergeration test added in the patch run.  
 
 
 Thanks,
 
 kturner

Re: [VOTE][LAZY] Format all supported branches

In the long run, the rules will (hopefully) save me work following up
fixing bad javadocs and trivial warnings.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 4:03 PM, Eric Newton eric.new...@gmail.com wrote:

 +0

 I don't think it's worth the disruption, but I don't mind if you're going
 to do all the work.


 On Wed, Jan 7, 2015 at 3:47 PM, Mike Drob mad...@cloudera.com wrote:

  Also, if you're using Eclipse to do the auto-format, please check for
  trailing white-space on otherwise empty javadoc lines.
 
  If you automate this in some fashion outside of Eclipse (because other
  people may prefer other editors), this would be a useful script to add
 to a
  top-level dev-tools folder.
 
  On Wed, Jan 7, 2015 at 2:36 PM, David Medinets david.medin...@gmail.com
 
  wrote:
 
   +1
  
   Are you automating the process so that you can re-apply the same steps
   in one year?
  
   On Wed, Jan 7, 2015 at 3:31 PM, Christopher ctubb...@apache.org
 wrote:
Can do. It's a bit more work for me, because I have to repeat the
 same
actions over and over again, but if it helps history look a little
   cleaner,
i can do it, and just stick to -sours and repeat for the next
 branch..
   
   
--
Christopher L Tubbs II
http://gravatar.com/ctubbsii
   
On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com
 wrote:
   
Please do not do formatting during merge conflict resolution, and
 make
those be separate commits.
   
On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser josh.el...@gmail.com
   wrote:
   
 ack'ed


 John Vines wrote:

 +1

 On Wed, Jan 7, 2015 at 3:12 PM, Christopherctubb...@apache.org
wrote:

  To make it easier to apply some minimal checkstyle rules for
 ACCUMULO-3451,
 I'm announcing my intentions to do a full, one-time, auto-format
  and
 organize imports on all our supported branches (1.5, 1.6, and
   master)
to
 bring us up to some degree of compliance with our agreed-upon
formatting
 standards.

 Benefits:
 To have additional checks, in particular against javadoc
 problems
   and
 other
 common trivial warnings in the build.
 To ensure less divergence from our agreed-upon formatting
  standards.
 Formatting first makes it much less tedious and easier on me to
  add
these
 checks to the build.

 Issues I've considered:
 I will deal with all the merge conflicts.
 I will ignore generated thrift code.
 Conflicts with new code in people's branches should be minimal
  (and
 easily
 resolved by formatting according to our standards).
 Regarding concerns about history tracking, in general, each
 format
change
 is small, but they are numerous. So, the impact on tracking
  history
 should
 be very minimal (you'll see things like a brace moved to the
 same
   line
as
 the else statement it is associated with... stuff that won't
   generally
 affect your ability to debug).
 I'll also do a format only commit, separately from any
  substantive
 changes regarding the rule changes, so the mass formatting
 change
   will
 happen in one place, and it will also be easy to revert, if
   absolutely
 necessary.

 I'll give this 24 hours (it can be reverted if somebody objects
   after
 that).

 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

Basically, hard enforcement (failing the build) makes us share
responsibility for quality control.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 4:28 PM, Christopher ctubb...@apache.org wrote:

 Fail (which is what I think we want, especially for the broken javadoc
 issues I've been seeing), but it could be configured either way. What I
 wouldn't want to happen is for them to be printed and simply ignored. If
 we're going to have to go back and fix them (instead of ignoring), I'd
 rather they fail, so the person who introduced the problem is held
 responsible for fixing before they push. The plugin can also be skipped, or
 configured to not fail, with command-line options.

 One great thing about Checkstyle rules, is they have *very* good
 descriptions about what is wrong. So, when you do see an error, it is
 typically a very obvious fix... and they give you the line number, too.


 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

 On Wed, Jan 7, 2015 at 4:12 PM, Mike Drob mad...@cloudera.com wrote:

 Will the checkstyle rules fail the build or just print warnings?

 On Wed, Jan 7, 2015 at 3:11 PM, Christopher ctubb...@apache.org wrote:

  In the long run, the rules will (hopefully) save me work following up
  fixing bad javadocs and trivial warnings.
 
 
  --
  Christopher L Tubbs II
  http://gravatar.com/ctubbsii
 
  On Wed, Jan 7, 2015 at 4:03 PM, Eric Newton eric.new...@gmail.com
 wrote:
 
   +0
  
   I don't think it's worth the disruption, but I don't mind if you're
 going
   to do all the work.
  
  
   On Wed, Jan 7, 2015 at 3:47 PM, Mike Drob mad...@cloudera.com
 wrote:
  
Also, if you're using Eclipse to do the auto-format, please check
 for
trailing white-space on otherwise empty javadoc lines.
   
If you automate this in some fashion outside of Eclipse (because
 other
people may prefer other editors), this would be a useful script to
 add
   to a
top-level dev-tools folder.
   
On Wed, Jan 7, 2015 at 2:36 PM, David Medinets 
  david.medin...@gmail.com
   
wrote:
   
 +1

 Are you automating the process so that you can re-apply the same
  steps
 in one year?

 On Wed, Jan 7, 2015 at 3:31 PM, Christopher ctubb...@apache.org
   wrote:
  Can do. It's a bit more work for me, because I have to repeat
 the
   same
  actions over and over again, but if it helps history look a
 little
 cleaner,
  i can do it, and just stick to -sours and repeat for the next
   branch..
 
 
  --
  Christopher L Tubbs II
  http://gravatar.com/ctubbsii
 
  On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com
   wrote:
 
  Please do not do formatting during merge conflict resolution,
 and
   make
  those be separate commits.
 
  On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser 
 josh.el...@gmail.com
 wrote:
 
   ack'ed
  
  
   John Vines wrote:
  
   +1
  
   On Wed, Jan 7, 2015 at 3:12 PM, Christopher
  ctubb...@apache.org
  wrote:
  
To make it easier to apply some minimal checkstyle rules
 for
   ACCUMULO-3451,
   I'm announcing my intentions to do a full, one-time,
  auto-format
and
   organize imports on all our supported branches (1.5, 1.6,
 and
 master)
  to
   bring us up to some degree of compliance with our
 agreed-upon
  formatting
   standards.
  
   Benefits:
   To have additional checks, in particular against javadoc
   problems
 and
   other
   common trivial warnings in the build.
   To ensure less divergence from our agreed-upon formatting
standards.
   Formatting first makes it much less tedious and easier on
 me
  to
add
  these
   checks to the build.
  
   Issues I've considered:
   I will deal with all the merge conflicts.
   I will ignore generated thrift code.
   Conflicts with new code in people's branches should be
 minimal
(and
   easily
   resolved by formatting according to our standards).
   Regarding concerns about history tracking, in general, each
   format
  change
   is small, but they are numerous. So, the impact on tracking
history
   should
   be very minimal (you'll see things like a brace moved to
 the
   same
 line
  as
   the else statement it is associated with... stuff that
 won't
 generally
   affect your ability to debug).
   I'll also do a format only commit, separately from any
substantive
   changes regarding the rule changes, so the mass formatting
   change
 will
   happen in one place, and it will also be easy to revert, if
 absolutely
   necessary.
  
   I'll give this 24 hours (it can be reverted if somebody
  objects
 after
   that).
  
   --
   Christopher L Tubbs II
   http://gravatar.com/ctubbsii

Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos



 On Dec. 29, 2014, 4:39 p.m., Josh Elser wrote:
  core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java,
   line 78
  https://reviews.apache.org/r/29386/diff/2/?file=802488#file802488line78
 
  To set up the handshake with a server, the client needs to know the 
  'instance' from the server's kerberos principal (accumulo in 
  accumulo/localhost@MY_DOMAIN). Reusing GENERAL_KERBEROS_PRINCIPAL (which 
  is the whole accumulo/host@DOMAIN value) lets use not have to introduce a 
  new property.
  
  However, reusing that parameter does force the client to know more 
  than it really needs to (it doesn't have to know host@DOMAIN, 
  explicitly). I'm not sure if that's an issue or not.

I introduced kerberos.server.primary and kerberos.server.realm to the client 
configuration to work around this. When converting from client config to an 
Accumulo config, the values are converted into general.kerberos.principal (with 
the `_HOST` expansion string as the principal's instance), and vice-versa. This 
should be seamless for user whichever value they use -- we can eventually make 
sure general.kerberos.principal only lives on the server.


- Josh


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


On Jan. 7, 2015, 9:37 p.m., Josh Elser wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/29386/
 ---
 
 (Updated Jan. 7, 2015, 9:37 p.m.)
 
 
 Review request for accumulo.
 
 
 Bugs: ACCUMULO-2815
 https://issues.apache.org/jira/browse/ACCUMULO-2815
 
 
 Repository: accumulo
 
 
 Description
 ---
 
 ACCUMULO-2815 Initial support for Kerberos client authentication.
 
 Leverage SASL transport provided by Thrift which can speak GSSAPI, which 
 Kerberos implements. Introduced...
 
 * An Accumulo KerberosToken which is an AuthenticationToken to validate users.
 * Custom thrift processor and invocation handler to ensure server RPCs have a 
 valid KRB identity and Accumulo authentication.
 * A KerberosAuthenticator which extends ZKAuthenticator to support Kerberos 
 identities seamlessly.
 * New ClientConf variables to use SASL transport and pass Kerberos server 
 principal
 * Updated ClientOpts and Shell opts to transparently use a KerberosToken when 
 SASL is enabled (no extra client work).
 
 I believe this is the bare minimum for Kerberos support. They are also 
 grossly lacking in unit and integration tests. I believe that I might have 
 somehow broken the client address string in the server (I saw log messages 
 with client: null, but I'm not sure if it's due to these changes or not). A 
 necessary limitation in the Thrift server used is that, like the SSL 
 transport, the SASL transport cannot presently be used with the 
 TFramedTransport, which means none of the [half]async thrift servers will 
 function with this -- we're stuck with the TThreadPoolServer.
 
 Performed some contrived benchmarks on my laptop (while still using it 
 myself) to get at big-picture view of the performance impact against normal 
 operation and Kerberos alone. Each run was the duration to ingest 100M 
 records using continuous-ingest, timed with `time`, using 'real'.
 
 THsHaServer (our default), 6 runs:
 
 Avg: 10m7.273s (607.273s)
 Min: 9m43.395s
 Max: 10m52.715s
 
 TThreadPoolServer (no SASL), 5 runs:
 
 Avg: 11m16.254s (676.254s)
 Min: 10m30.987s
 Max: 12m24.192s
 
 TThreadPoolServer+SASL/GSSAPI (these changes), 6 runs:
 
 Avg: 13m17.187s (797.187s)
 Min: 10m52.997s
 Max: 16m0.975s
 
 The general takeway is that there's about 15% performance degredation in its 
 initial state which is in the realm of what I expected (~10%).
 
 
 Diffs
 -
 
   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java f6ea934 
   core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java 
 6fe61a5 
   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
 e75bec6 
   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
 f481cc3 
   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
 a9ad8a1 
   
 core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java
  6dc846f 
   
 core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java
  5da803b 
   
 core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java
  PRE-CREATION 
   core/src/main/java/org/apache/accumulo/core/conf/Property.java e054a5f 
   core/src/main/java/org/apache/accumulo/core/rpc/FilterTransport.java 
 PRE-CREATION 
   core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java 
 PRE-CREATION

Re: [VOTE][LAZY] Format all supported branches

Fail (which is what I think we want, especially for the broken javadoc
issues I've been seeing), but it could be configured either way. What I
wouldn't want to happen is for them to be printed and simply ignored. If
we're going to have to go back and fix them (instead of ignoring), I'd
rather they fail, so the person who introduced the problem is held
responsible for fixing before they push. The plugin can also be skipped, or
configured to not fail, with command-line options.

One great thing about Checkstyle rules, is they have *very* good
descriptions about what is wrong. So, when you do see an error, it is
typically a very obvious fix... and they give you the line number, too.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 4:12 PM, Mike Drob mad...@cloudera.com wrote:

 Will the checkstyle rules fail the build or just print warnings?

 On Wed, Jan 7, 2015 at 3:11 PM, Christopher ctubb...@apache.org wrote:

  In the long run, the rules will (hopefully) save me work following up
  fixing bad javadocs and trivial warnings.
 
 
  --
  Christopher L Tubbs II
  http://gravatar.com/ctubbsii
 
  On Wed, Jan 7, 2015 at 4:03 PM, Eric Newton eric.new...@gmail.com
 wrote:
 
   +0
  
   I don't think it's worth the disruption, but I don't mind if you're
 going
   to do all the work.
  
  
   On Wed, Jan 7, 2015 at 3:47 PM, Mike Drob mad...@cloudera.com wrote:
  
Also, if you're using Eclipse to do the auto-format, please check for
trailing white-space on otherwise empty javadoc lines.
   
If you automate this in some fashion outside of Eclipse (because
 other
people may prefer other editors), this would be a useful script to
 add
   to a
top-level dev-tools folder.
   
On Wed, Jan 7, 2015 at 2:36 PM, David Medinets 
  david.medin...@gmail.com
   
wrote:
   
 +1

 Are you automating the process so that you can re-apply the same
  steps
 in one year?

 On Wed, Jan 7, 2015 at 3:31 PM, Christopher ctubb...@apache.org
   wrote:
  Can do. It's a bit more work for me, because I have to repeat the
   same
  actions over and over again, but if it helps history look a
 little
 cleaner,
  i can do it, and just stick to -sours and repeat for the next
   branch..
 
 
  --
  Christopher L Tubbs II
  http://gravatar.com/ctubbsii
 
  On Wed, Jan 7, 2015 at 3:25 PM, Mike Drob mad...@cloudera.com
   wrote:
 
  Please do not do formatting during merge conflict resolution,
 and
   make
  those be separate commits.
 
  On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser 
 josh.el...@gmail.com
 wrote:
 
   ack'ed
  
  
   John Vines wrote:
  
   +1
  
   On Wed, Jan 7, 2015 at 3:12 PM, Christopher
  ctubb...@apache.org
  wrote:
  
To make it easier to apply some minimal checkstyle rules for
   ACCUMULO-3451,
   I'm announcing my intentions to do a full, one-time,
  auto-format
and
   organize imports on all our supported branches (1.5, 1.6,
 and
 master)
  to
   bring us up to some degree of compliance with our
 agreed-upon
  formatting
   standards.
  
   Benefits:
   To have additional checks, in particular against javadoc
   problems
 and
   other
   common trivial warnings in the build.
   To ensure less divergence from our agreed-upon formatting
standards.
   Formatting first makes it much less tedious and easier on me
  to
add
  these
   checks to the build.
  
   Issues I've considered:
   I will deal with all the merge conflicts.
   I will ignore generated thrift code.
   Conflicts with new code in people's branches should be
 minimal
(and
   easily
   resolved by formatting according to our standards).
   Regarding concerns about history tracking, in general, each
   format
  change
   is small, but they are numerous. So, the impact on tracking
history
   should
   be very minimal (you'll see things like a brace moved to the
   same
 line
  as
   the else statement it is associated with... stuff that won't
 generally
   affect your ability to debug).
   I'll also do a format only commit, separately from any
substantive
   changes regarding the rule changes, so the mass formatting
   change
 will
   happen in one place, and it will also be easy to revert, if
 absolutely
   necessary.
  
   I'll give this 24 hours (it can be reverted if somebody
  objects
 after
   that).
  
   --
   Christopher L Tubbs II
   http://gravatar.com/ctubbsii

Re: [VOTE][LAZY] Format all supported branches

+1 for that. I feel bad when I see you constantly come across after my 
commits when I inevitably bork some Javadoc.


Christopher wrote:

Basically, hard enforcement (failing the build) makes us share
responsibility for quality control.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 4:28 PM, Christopherctubb...@apache.org  wrote:


Fail (which is what I think we want, especially for the broken javadoc
issues I've been seeing), but it could be configured either way. What I
wouldn't want to happen is for them to be printed and simply ignored. If
we're going to have to go back and fix them (instead of ignoring), I'd
rather they fail, so the person who introduced the problem is held
responsible for fixing before they push. The plugin can also be skipped, or
configured to not fail, with command-line options.

One great thing about Checkstyle rules, is they have *very* good
descriptions about what is wrong. So, when you do see an error, it is
typically a very obvious fix... and they give you the line number, too.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 4:12 PM, Mike Drobmad...@cloudera.com  wrote:


Will the checkstyle rules fail the build or just print warnings?

On Wed, Jan 7, 2015 at 3:11 PM, Christopherctubb...@apache.org  wrote:


In the long run, the rules will (hopefully) save me work following up
fixing bad javadocs and trivial warnings.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 4:03 PM, Eric Newtoneric.new...@gmail.com

wrote:

+0

I don't think it's worth the disruption, but I don't mind if you're

going

to do all the work.


On Wed, Jan 7, 2015 at 3:47 PM, Mike Drobmad...@cloudera.com

wrote:

Also, if you're using Eclipse to do the auto-format, please check

for

trailing white-space on otherwise empty javadoc lines.

If you automate this in some fashion outside of Eclipse (because

other

people may prefer other editors), this would be a useful script to

add

to a

top-level dev-tools folder.

On Wed, Jan 7, 2015 at 2:36 PM, David Medinets

david.medin...@gmail.com

wrote:


+1

Are you automating the process so that you can re-apply the same

steps

in one year?

On Wed, Jan 7, 2015 at 3:31 PM, Christopherctubb...@apache.org

wrote:

Can do. It's a bit more work for me, because I have to repeat

the

same

actions over and over again, but if it helps history look a

little

cleaner,

i can do it, and just stick to -sours and repeat for the next

branch..


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 3:25 PM, Mike Drobmad...@cloudera.com

wrote:

Please do not do formatting during merge conflict resolution,

and

make

those be separate commits.

On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser

josh.el...@gmail.com

wrote:

ack'ed


John Vines wrote:


+1

On Wed, Jan 7, 2015 at 3:12 PM, Christopher

ctubb...@apache.org

wrote:

  To make it easier to apply some minimal checkstyle rules

for

ACCUMULO-3451,
I'm announcing my intentions to do a full, one-time,

auto-format

and

organize imports on all our supported branches (1.5, 1.6,

and

master)

to

bring us up to some degree of compliance with our

agreed-upon

formatting

standards.

Benefits:
To have additional checks, in particular against javadoc

problems

and

other
common trivial warnings in the build.
To ensure less divergence from our agreed-upon formatting

standards.

Formatting first makes it much less tedious and easier on

me

to

add

these

checks to the build.

Issues I've considered:
I will deal with all the merge conflicts.
I will ignore generated thrift code.
Conflicts with new code in people's branches should be

minimal

(and

easily
resolved by formatting according to our standards).
Regarding concerns about history tracking, in general, each

format

change

is small, but they are numerous. So, the impact on tracking

history

should
be very minimal (you'll see things like a brace moved to

the

same

line

as

the else statement it is associated with... stuff that

won't

generally

affect your ability to debug).
I'll also do a format only commit, separately from any

substantive

changes regarding the rule changes, so the mass formatting

change

will

happen in one place, and it will also be easy to revert, if

absolutely

necessary.

I'll give this 24 hours (it can be reverted if somebody

objects

after

that).

--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos


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

(Updated Jan. 7, 2015, 9:37 p.m.)


Review request for accumulo.


Changes
---

Uploaded the wrong patch for the last revision. This actually contains the 
changes I said I made last time.


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


Repository: accumulo


Description
---

ACCUMULO-2815 Initial support for Kerberos client authentication.

Leverage SASL transport provided by Thrift which can speak GSSAPI, which 
Kerberos implements. Introduced...

* An Accumulo KerberosToken which is an AuthenticationToken to validate users.
* Custom thrift processor and invocation handler to ensure server RPCs have a 
valid KRB identity and Accumulo authentication.
* A KerberosAuthenticator which extends ZKAuthenticator to support Kerberos 
identities seamlessly.
* New ClientConf variables to use SASL transport and pass Kerberos server 
principal
* Updated ClientOpts and Shell opts to transparently use a KerberosToken when 
SASL is enabled (no extra client work).

I believe this is the bare minimum for Kerberos support. They are also 
grossly lacking in unit and integration tests. I believe that I might have 
somehow broken the client address string in the server (I saw log messages with 
client: null, but I'm not sure if it's due to these changes or not). A 
necessary limitation in the Thrift server used is that, like the SSL transport, 
the SASL transport cannot presently be used with the TFramedTransport, which 
means none of the [half]async thrift servers will function with this -- we're 
stuck with the TThreadPoolServer.

Performed some contrived benchmarks on my laptop (while still using it myself) 
to get at big-picture view of the performance impact against normal operation 
and Kerberos alone. Each run was the duration to ingest 100M records using 
continuous-ingest, timed with `time`, using 'real'.

THsHaServer (our default), 6 runs:

Avg: 10m7.273s (607.273s)
Min: 9m43.395s
Max: 10m52.715s

TThreadPoolServer (no SASL), 5 runs:

Avg: 11m16.254s (676.254s)
Min: 10m30.987s
Max: 12m24.192s

TThreadPoolServer+SASL/GSSAPI (these changes), 6 runs:

Avg: 13m17.187s (797.187s)
Min: 10m52.997s
Max: 16m0.975s

The general takeway is that there's about 15% performance degredation in its 
initial state which is in the realm of what I expected (~10%).


Diffs (updated)
-

  core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java f6ea934 
  core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java 
6fe61a5 
  core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
e75bec6 
  core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
f481cc3 
  core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
a9ad8a1 
  
core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java 
6dc846f 
  
core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java
 5da803b 
  
core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java
 PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/conf/Property.java e054a5f 
  core/src/main/java/org/apache/accumulo/core/rpc/FilterTransport.java 
PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java 
PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java 
6eace77 
  core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java 09bd6c4 
  core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java 
PRE-CREATION 
  
core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransportFactory.java
 PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/security/Credentials.java 525a958 
  core/src/test/java/org/apache/accumulo/core/cli/TestClientOpts.java ff49bc0 
  
core/src/test/java/org/apache/accumulo/core/client/ClientConfigurationTest.java 
PRE-CREATION 
  
core/src/test/java/org/apache/accumulo/core/client/impl/ThriftTransportKeyTest.java
 PRE-CREATION 
  core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java 
40be70f 
  core/src/test/java/org/apache/accumulo/core/rpc/SaslConnectionParamsTest.java 
PRE-CREATION 
  
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java
 27d6b19 
  
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java
 26c23ed 
  pom.xml ae188a0 
  proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java 4b048eb 
  
server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java 
09ae4f4 
  server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 
046cfb5 
  
server/base/src/main/java/org/apache/accumulo/server/rpc/TCredentialsUpdatingInvocationHandler.java

Re: [VOTE][LAZY] Format all supported branches

Some of the checkstyle rules catch stuff in javadocs, which even I don't
typically think to look for (like invalid HTML), and I'm pretty pedantic
already when it comes to javadocs.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Wed, Jan 7, 2015 at 4:36 PM, Josh Elser josh.el...@gmail.com wrote:

 +1 for that. I feel bad when I see you constantly come across after my
 commits when I inevitably bork some Javadoc.


 Christopher wrote:

 Basically, hard enforcement (failing the build) makes us share
 responsibility for quality control.


 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

 On Wed, Jan 7, 2015 at 4:28 PM, Christopherctubb...@apache.org  wrote:

  Fail (which is what I think we want, especially for the broken javadoc
 issues I've been seeing), but it could be configured either way. What I
 wouldn't want to happen is for them to be printed and simply ignored. If
 we're going to have to go back and fix them (instead of ignoring), I'd
 rather they fail, so the person who introduced the problem is held
 responsible for fixing before they push. The plugin can also be skipped,
 or
 configured to not fail, with command-line options.

 One great thing about Checkstyle rules, is they have *very* good
 descriptions about what is wrong. So, when you do see an error, it is
 typically a very obvious fix... and they give you the line number, too.


 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

 On Wed, Jan 7, 2015 at 4:12 PM, Mike Drobmad...@cloudera.com  wrote:

  Will the checkstyle rules fail the build or just print warnings?

 On Wed, Jan 7, 2015 at 3:11 PM, Christopherctubb...@apache.org
 wrote:

  In the long run, the rules will (hopefully) save me work following up
 fixing bad javadocs and trivial warnings.


 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

 On Wed, Jan 7, 2015 at 4:03 PM, Eric Newtoneric.new...@gmail.com

 wrote:

 +0

 I don't think it's worth the disruption, but I don't mind if you're

 going

 to do all the work.


 On Wed, Jan 7, 2015 at 3:47 PM, Mike Drobmad...@cloudera.com

 wrote:

 Also, if you're using Eclipse to do the auto-format, please check

 for

 trailing white-space on otherwise empty javadoc lines.

 If you automate this in some fashion outside of Eclipse (because

 other

 people may prefer other editors), this would be a useful script to

 add

 to a

 top-level dev-tools folder.

 On Wed, Jan 7, 2015 at 2:36 PM, David Medinets

 david.medin...@gmail.com

 wrote:

  +1

 Are you automating the process so that you can re-apply the same

 steps

 in one year?

 On Wed, Jan 7, 2015 at 3:31 PM, Christopherctubb...@apache.org

 wrote:

 Can do. It's a bit more work for me, because I have to repeat

 the

 same

 actions over and over again, but if it helps history look a

 little

 cleaner,

 i can do it, and just stick to -sours and repeat for the next

 branch..


 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

 On Wed, Jan 7, 2015 at 3:25 PM, Mike Drobmad...@cloudera.com

 wrote:

 Please do not do formatting during merge conflict resolution,

 and

 make

 those be separate commits.

 On Wed, Jan 7, 2015 at 2:23 PM, Josh Elser

 josh.el...@gmail.com

 wrote:

 ack'ed


 John Vines wrote:

  +1

 On Wed, Jan 7, 2015 at 3:12 PM, Christopher

 ctubb...@apache.org

 wrote:

   To make it easier to apply some minimal checkstyle rules

 for

 ACCUMULO-3451,
 I'm announcing my intentions to do a full, one-time,

 auto-format

 and

 organize imports on all our supported branches (1.5, 1.6,

 and

 master)

 to

 bring us up to some degree of compliance with our

 agreed-upon

 formatting

 standards.

 Benefits:
 To have additional checks, in particular against javadoc

 problems

 and

 other
 common trivial warnings in the build.
 To ensure less divergence from our agreed-upon formatting

 standards.

 Formatting first makes it much less tedious and easier on

 me

 to

 add

 these

 checks to the build.

 Issues I've considered:
 I will deal with all the merge conflicts.
 I will ignore generated thrift code.
 Conflicts with new code in people's branches should be

 minimal

 (and

 easily
 resolved by formatting according to our standards).
 Regarding concerns about history tracking, in general, each

 format

 change

 is small, but they are numerous. So, the impact on tracking

 history

 should
 be very minimal (you'll see things like a brace moved to

 the

 same

 line

 as

 the else statement it is associated with... stuff that

 won't

 generally

 affect your ability to debug).
 I'll also do a format only commit, separately from any

 substantive

 changes regarding the rule changes, so the mass formatting

 change

 will

 happen in one place, and it will also be easy to revert, if

 absolutely

 necessary.

 I'll give this 24 hours (it can be reverted if somebody

 objects

 after

 that).

 --
 Christopher L Tubbs II
 http://gravatar.com/ctubbsii

Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos

2015-01-07 Thread Christopher Tubbs

On Dec. 30, 2014, 5:46 p.m., Christopher Tubbs wrote:
server/base/src/main/java/org/apache/accumulo/server/security/handler/KerberosAuthenticator.java,
line 41
https://reviews.apache.org/r/29386/diff/4/?file=803164#file803164line41

I don't think this should be so tightly coupled with the
ZKAuthenticator. It doesn't seem that this coupling is necessary to provide
authentication functionality.

Have a different implementation of an Authenticator that hijacks the
data storage of another implementation, could be quite confusing to
administrators, especially if they need to swap out the implementation.

See ACCUMULO-1300 and related issues for other options.

Josh Elser wrote:
I understand the intentions for ACCUMULO-1300; however, that's not
implemented. Being able to leverage the existing ZKAuthorizor was wonderful
so I really don't want to move away from how this fundamentally works.

Shouldn't the Authenticator and Authorizor be fixed per instance
(allowing reset via `accumulo init --reset-security` or w/e)? What do you
actually want changed aside from documentation to make it clear to
administrators (I haven't written any documentation yet).

Christopher Tubbs wrote:
So, I pointed out in the conversation around ACCUMULO-259, that I would
prefer a pluggable security module that integrated all the security-related
functions, because they are so dependent on each other. However, they were
implemented as 3 separately configurable pluggable components. I would still
like to see them merged (which doesn't preclude making one from modular
components themselves).

For now, I'm not really sure what it means for somebody to create a
user when the authenticator already validates them. That's why it's
confusing. Are we essentially making ZKAuthenticator Kerberos-aware, or are
we providing a separate Kerberos Authenticator? That's really what it boils
down to for me. It seems like we're doing a hybrid... but I think we should
solidify on one or the other.

Josh Elser wrote:
I view it as a Kerberos Authenticator. The implementation details of that
authenticator is that is happens to persist information into ZK to handle
things like permissions and authorizations, but a user doesn't have to know
that to use it. We could, hypothetically, swap out ZK for any other
persistent store for that information and it would continue to work.

What would help alleviate your confusion? More javadocs? A different
class name?

Christopher Tubbs wrote:
Well, part is documentation. Part is behavior.

If it's a separate authenticator, we should make sure that it does not
use the same storage area in such a tightly coupled way. It should not, for
instance, be setting things that look like passwords, and create local
user, list local users, and delete local users should be unsupported
client side operations, just as you've done with change password (since
users are managed within Kerberos). We already share storage between the
ZKAuthenticator and the corresponding permissions handler and authorizer, so
it's probably okay to share the directory identified with the principal.
However, it can seamlessly create this directory as needed.

Mainly, I'm thinking the more we can separate the local user management
functions from the KerberosAuthenticator, the better, since users are managed
in Kerberos. Some of my concerns are with the current state of things, but
I'm hoping not to exacerbate the problems I see with the current
implementations. I don't want to make it harder to make improvements later.
I'd rather move towards a better design, then simply add features to the
existing ad-hoc design.

Josh Elser wrote:
bq. It should not, for instance, be setting things that look like
passwords, and create local user, list local users, and delete local
users should be unsupported client side operations, just as you've done with
change password (since users are managed within Kerberos)

Thanks for pointing that out. That helps me understand where your
concerns are coming from. Perhaps it would be cleaner to just keep a
reference to the ZkAuthenticator as a delegate (instead of extending it).
This would give us a bit more obvious control over what's happening.

bq. I'm thinking the more we can separate the local user management
functions from the KerberosAuthenticator, the better, since users are managed
in Kerberos. Some of my concerns are with the current state of things, but
I'm hoping not to exacerbate the problems I see with the current
implementations

Agreed, those are very.. awkward, presently. Is making sure that we throw
exceptions from KerberosAuthenticator when we call these non-sensical methods
a sufficient change? Then, we can move to a better design later on like you
said?

Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos


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

(Updated Jan. 7, 2015, 7:20 p.m.)


Review request for accumulo.


Changes
---

Cleanup of the KerberosAuthenticator (remove random password and create base 
znodes for permhandler and authorizor) and adds some more tests.


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


Repository: accumulo


Description
---

ACCUMULO-2815 Initial support for Kerberos client authentication.

Leverage SASL transport provided by Thrift which can speak GSSAPI, which 
Kerberos implements. Introduced...

* An Accumulo KerberosToken which is an AuthenticationToken to validate users.
* Custom thrift processor and invocation handler to ensure server RPCs have a 
valid KRB identity and Accumulo authentication.
* A KerberosAuthenticator which extends ZKAuthenticator to support Kerberos 
identities seamlessly.
* New ClientConf variables to use SASL transport and pass Kerberos server 
principal
* Updated ClientOpts and Shell opts to transparently use a KerberosToken when 
SASL is enabled (no extra client work).

I believe this is the bare minimum for Kerberos support. They are also 
grossly lacking in unit and integration tests. I believe that I might have 
somehow broken the client address string in the server (I saw log messages with 
client: null, but I'm not sure if it's due to these changes or not). A 
necessary limitation in the Thrift server used is that, like the SSL transport, 
the SASL transport cannot presently be used with the TFramedTransport, which 
means none of the [half]async thrift servers will function with this -- we're 
stuck with the TThreadPoolServer.

Performed some contrived benchmarks on my laptop (while still using it myself) 
to get at big-picture view of the performance impact against normal operation 
and Kerberos alone. Each run was the duration to ingest 100M records using 
continuous-ingest, timed with `time`, using 'real'.

THsHaServer (our default), 6 runs:

Avg: 10m7.273s (607.273s)
Min: 9m43.395s
Max: 10m52.715s

TThreadPoolServer (no SASL), 5 runs:

Avg: 11m16.254s (676.254s)
Min: 10m30.987s
Max: 12m24.192s

TThreadPoolServer+SASL/GSSAPI (these changes), 6 runs:

Avg: 13m17.187s (797.187s)
Min: 10m52.997s
Max: 16m0.975s

The general takeway is that there's about 15% performance degredation in its 
initial state which is in the realm of what I expected (~10%).


Diffs (updated)
-

  core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java f6ea934 
  core/src/main/java/org/apache/accumulo/core/client/ClientConfiguration.java 
6fe61a5 
  core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
e75bec6 
  core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
f481cc3 
  core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
a9ad8a1 
  
core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java 
6dc846f 
  
core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java
 5da803b 
  
core/src/main/java/org/apache/accumulo/core/client/security/tokens/KerberosToken.java
 PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/conf/Property.java e054a5f 
  core/src/main/java/org/apache/accumulo/core/rpc/FilterTransport.java 
PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/rpc/SaslConnectionParams.java 
PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/rpc/TTimeoutTransport.java 
6eace77 
  core/src/main/java/org/apache/accumulo/core/rpc/ThriftUtil.java 09bd6c4 
  core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransport.java 
PRE-CREATION 
  
core/src/main/java/org/apache/accumulo/core/rpc/UGIAssumingTransportFactory.java
 PRE-CREATION 
  core/src/main/java/org/apache/accumulo/core/security/Credentials.java 525a958 
  core/src/test/java/org/apache/accumulo/core/cli/TestClientOpts.java ff49bc0 
  
core/src/test/java/org/apache/accumulo/core/client/ClientConfigurationTest.java 
PRE-CREATION 
  
core/src/test/java/org/apache/accumulo/core/client/impl/ThriftTransportKeyTest.java
 PRE-CREATION 
  core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java 
40be70f 
  core/src/test/java/org/apache/accumulo/core/rpc/SaslConnectionParamsTest.java 
PRE-CREATION 
  
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java
 27d6b19 
  
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java
 26c23ed 
  pom.xml ae188a0 
  proxy/src/main/java/org/apache/accumulo/proxy/Proxy.java 4b048eb 
  
server/base/src/main/java/org/apache/accumulo/server/AccumuloServerContext.java 
09ae4f4 
  server/base/src/main/java/org/apache/accumulo/server/init/Initialize.java 
046cfb5

Re: Review Request 29386: ACCUMULO-2815 Client authentication via Kerberos