----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30301/#review70457 -----------------------------------------------------------
This generally looks good. I posted some comments through the code. Once those are fixed, I will review the tests and we can take it from there. flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java <https://reviews.apache.org/r/30301/#comment115657> This whole thing should happen inside if(enableSSL). There is no point trying to read conf values that would never be used if ssl is false. flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java <https://reviews.apache.org/r/30301/#comment115658> Is this in seconds or millis? Also, why was 10000 chosen? If it is in millis we should probably bump it up. flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java <https://reviews.apache.org/r/30301/#comment115659> This code is the same as the one in the catch block below. This should be abstracted out into a method which takes the serverTransport instance as a parameter flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java <https://reviews.apache.org/r/30301/#comment115660> Same as above. These lookups need to happen only if ssl is enabled. flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java <https://reviews.apache.org/r/30301/#comment115661> Taken or Lifted? ;-) flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java <https://reviews.apache.org/r/30301/#comment115663> Again, we should have a higher timeout flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java <https://reviews.apache.org/r/30301/#comment115664> if(..) { .. } flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java <https://reviews.apache.org/r/30301/#comment115667> This should be FlumeException not a TTransportException flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java <https://reviews.apache.org/r/30301/#comment115669> This entire class seems like overkill. Since we are creating the socket from the SSLSocketFactory ourselves and then passing it on to thrift, why not simply create the socket and then exclude the protocols and then pass the socket to the TSocket constructor? - Hari Shreedharan On Jan. 27, 2015, 1:54 a.m., Johny Rufus John wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30301/ > ----------------------------------------------------------- > > (Updated Jan. 27, 2015, 1:54 a.m.) > > > Review request for Flume. > > > Bugs: FLUME-2574 > https://issues.apache.org/jira/browse/FLUME-2574 > > > Repository: flume-git > > > Description > ------- > > Current Thrift Source/Sink does not have support for SSL. Similar to Avro we > should start supporting SSL for Thrift based communication > > > Diffs > ----- > > flume-ng-core/src/main/java/org/apache/flume/source/ThriftSource.java > 551fe13 > flume-ng-core/src/test/java/org/apache/flume/sink/TestThriftSink.java > fccaede > flume-ng-core/src/test/java/org/apache/flume/source/TestThriftSource.java > 357965f > flume-ng-core/src/test/resources/keystorefile.jks PRE-CREATION > flume-ng-core/src/test/resources/truststorefile.jks PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/ThriftRpcClient.java > 6382a0e > flume-ng-sdk/src/test/java/org/apache/flume/api/ThriftTestingSource.java > 63d2fc3 > > Diff: https://reviews.apache.org/r/30301/diff/ > > > Testing > ------- > > mvn clean install -Dhadoop.profile=hbase-98 > > > Thanks, > > Johny Rufus John > >
