----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4380/#review6319 -----------------------------------------------------------
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13731> Yes..missed this one when I made that change. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13732> Agreed. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13733> getClient logic is if the client itself is null it calls getNextClient(). This call is in effect made to getNextClient, which I felt should only be called through getClient() which is a synchronized wrapper flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13734> agreed. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13735> Until and unless we decide whether FlumeException should be checked or not, I don't think we should be throwing only EventDeliveryExceptions everywhere. This is not a delivery exception, this is a "client is closed exception", so I think this is the right exception to throw. Javadocs show that this can throw FlumeException too. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13736> lastCheckedhost will be initialized at -1 i the updated patch, which will fix the calls issue. The logic is : From current location go to end of loop, and then move forward until you hit current location again. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13730> I didn't get this comment. We need the enum to figure out what client to build. We read the type from the properties and then create the Client instance using this. This helps make it extensible if we decide to add more RpcClients. So I can't see a way around it, other than a bunch of if-else-if. - Hari On 2012-03-22 03:43:56, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4380/ > ----------------------------------------------------------- > > (Updated 2012-03-22 03:43:56) > > > 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 > >
