----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4380/#review6467 -----------------------------------------------------------
Changes seem to be coming together well. Thanks for the prompt turnaround. Some feedback follows. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14090> Please add it to the top javadoc with explanation of how this property works. Also, perhaps better called max-attempts. In general, I suggest that these property names be defined at the top as constants that are then referenced within the code. For example: public static final String CONFIG_MAX_ATTEMPTS = "max-attempts"; flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14091> formatting: } else { // same line flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14092> formatting: } else { // same line flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14093> Please log the exception. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14097> What is the reason for catching this exception differently? Seems redundant. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14095> Please log the exception. Ex: logger.error("...", e2); flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14096> Looks like there is an off-by-one problem here. If the last try was successful, the exception will be still raised. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14099> Please log the exception. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14098> This should be catching java.lang.Exception instead of FlumeException. Otherwise you risk breaking the failover logic. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14100> log the exception. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment14102> What is the purpose of this? flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java <https://reviews.apache.org/r/4380/#comment14103> Please log the exception. flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java <https://reviews.apache.org/r/4380/#comment14104> log the exception. flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java <https://reviews.apache.org/r/4380/#comment14105> log the exception. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment14106> Alternatively you could cast the client instance to RpcClient and invoke the configure(properties) method on it directly. - Arvind On 2012-03-28 02:38:02, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4380/ > ----------------------------------------------------------- > > (Updated 2012-03-28 02:38:02) > > > 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/NettyAvroRpcClient.java > 965b2ff > 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/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 > >
