Review Board 1.7.22


FLUME-1628. Refactor HDFS Sink Kerberos auth methods to a public class

Review Request #7496 - Created Oct. 9, 2012 and updated

Hari Shreedharan
FLUME-1628
Reviewers
Flume
flume-git
Refactoring HDFS Sink's kerberos code to another class to make it available to Hbase sink etc.

 
Total:
3
Open:
0
Resolved:
2
Dropped:
1
Status:
From:
Description From Last Updated Status
Posted (Oct. 12, 2012, 12:07 a.m.)
I spent a little bit of time looking at this. Aside from some specific questions below, I have some general feedback:
1. Any reason why the Sink needs to be part of the API? Seems like it's just using it to get a string for an error message. If we want to keep that, we should pass the string as a component name or something. Mentioned below as a circular reference.
2. Has this been tested? Does it work? Unfortunately, there are no unit tests for Kerberos login or auth, mostly because it's not very easy to set up an environment for it.
3. I'm hesitant to target this for 1.3.0 since this functionality is not being used by the HBase Sink yet, so we would be exposing a new API that only one thing uses. We don't know if anything is missing that is needed by HBase.
  1. 1. Yep - can be fixed quite easily.
    2. No, I plan to test this soon though. This code is simply copy-pasted. So it should not break anything. I can test it soon though.
    3. I don't think this is necessary for 1.3.0, since I don't plan to get the Hbase stuff done by then anyway.
  2. It would probably be a decent size project but hadoop does have code to programmatically setup a kdc. Search for kdc on this pom https://github.com/apache/hadoop-common/blob/trunk/hadoop-common-project/hadoop-common/pom.xml
  3. Brock, good point. It would be very valuable for us to get that in. Agreed that it's likely a decent chunk of work.
We are creating a circular reference here
  1. Agreed, passing in the name is probably enough. 
Any drawback to returning this from the authenticate() call?
  1. Hmm..Yes. If security is not enabled, we still need to return true from authenticate() - else we cannot log error reliably(if we just check for null being returned from authenticate) to inform of login failure.
Probably want the sink name instead of "this" in the log statement
  1. Yep.  Will do that.