----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4380/#review6499 -----------------------------------------------------------
Ship it! +1. Thanks for incorporating the changes Hari. There is some suggestions below for you to consider. Please attach the patch to the Jira. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14159> please use a public static final String CONFIG_HOSTS = "hosts". flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment14157> Please make it public static final, and rename it CONF_CLIENT_TYPE instead. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment14153> Please reference the Enum directly. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment14156> Rename to getDefaultClientInstance(...) flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment14155> Rename to getDefaultClientInstance(...) flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment14151> This should be called DEFAULT flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment14152> Perhaps better called DEFAULT_FAILOVER - Arvind On 2012-03-28 07:25:01, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4380/ > ----------------------------------------------------------- > > (Updated 2012-03-28 07:25:01) > > > 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/AbstractRpcClient.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/RpcClient.java a601213 > 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 > >
