----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7496/#review12378 -----------------------------------------------------------
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. flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java <https://reviews.apache.org/r/7496/#comment26198> We are creating a circular reference here flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java <https://reviews.apache.org/r/7496/#comment26199> Any drawback to returning this from the authenticate() call? flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/KerberosLoginManager.java <https://reviews.apache.org/r/7496/#comment26197> Probably want the sink name instead of "this" in the log statement - Mike Percy On Oct. 9, 2012, 7:23 a.m., Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7496/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2012, 7:23 a.m.) > > > Review request for Flume. > > > Description > ------- > > Refactoring HDFS Sink's kerberos code to another class to make it available > to Hbase sink etc. > > > This addresses bug FLUME-1628. > https://issues.apache.org/jira/browse/FLUME-1628 > > > Diffs > ----- > > > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java > 5ec9eb8 > > flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/KerberosLoginManager.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/7496/diff/ > > > Testing > ------- > > > Thanks, > > Hari Shreedharan > >
