----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4380/#review6375 -----------------------------------------------------------
Thanks for the patch Hari. Some feedback follows: * General reminder that we need to follow the JCC style for formatting as specified in https://cwiki.apache.org/confluence/display/FLUME/How+to+Contribute#HowtoContribute-CodeQuality. Minimally, we need curly braces even if the code block is one line; else should be on the same line as the closing brace for the corresponding if statement; indentation should be aligned to the extent possible. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13845> This is redundant flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13859> The configuration specification should be in the class-level javadocs for clear visiblity. flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java <https://reviews.apache.org/r/4380/#comment14042> Exception should be logged. flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java <https://reviews.apache.org/r/4380/#comment14044> s/"Invalid port specified: " + hostAndProt[1] flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13897> Please use = instead of : to disambiguate since the value is also expected to use : char as a separator. Also, it is better to point this javadoc to the RpcClient implementations so that they can be the one place where configuration is defined. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13907> Please externalize this property in to a constants class or move it up top as a public static final String. Also suggest changing it to "clienttype" or "client.type". flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13910> This will certainly cause problems in environments that use a security manager. Instead I suggest refactoring the client API to allow for a configure(Properties) contract via interface that is invoked directly. Other options can work too. - Arvind On 2012-03-26 20:00:38, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4380/ > ----------------------------------------------------------- > > (Updated 2012-03-26 20:00:38) > > > Review request for Flume. > > > Summary > ------- > > Submitting an initial cut of FailoverRpcClient that uses the NettyRpcClient > under the hood. In this version, host selection is not exactly the best, > please make suggestions on how to improve it. As of now, the first version > will not have a backoff mechanism to not retry a host for a fixed time etc(as > discussed in the jira). I will add unit tests soon. > > Note that the actual "connect" call to a host is hidden from the > FailoverClient (by the Netty client or any other implementation, which we may > choose to use later). Since this connect call is hidden, failure to create a > client(the build function throwing an exception) is not being considered a > failure. Only a failure to append is considered a failure, and counted > towards the maximum number of tries. In other words, as far as the > FailoverClient(for that matter, any implementation of RpcClient interface) > would consider an append failure as failure, not a failure to a build() call > - if we want to make sure that a connect failure also is counted, we should > move the connect call to the append function and keep track of the connection > state internally, and not expect any code depending on an implementation of > RpcClient(including other clients which depend on pre-existing clients) to > know that a build call also creates a connection - this is exactly like a > socket implementation, creating a new socket does not initialize a > connection, it is done explicitly. > > > This addresses bug FLUME-962. > https://issues.apache.org/jira/browse/FLUME-962 > > > Diffs > ----- > > flume-ng-sdk/src/main/java/org/apache/flume/api/ClientType.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java > 965b2ff > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java > 351b5b1 > flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 93bfee9 > flume-ng-sdk/src/test/java/org/apache/flume/api/TestFailoverRpcClient.java > PRE-CREATION > flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java > a33e9c8 > flume-ng-sdk/src/test/java/org/apache/flume/api/TestRpcClientFactory.java > 0c94231 > > Diff: https://reviews.apache.org/r/4380/diff > > > Testing > ------- > > Unit tests added for the new functionality > > > Thanks, > > Hari > >
