----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4380/#review6089 -----------------------------------------------------------
Thanks for the patch! I've added some feedback on the API inline in the review. I mostly just have comments on the API, not on the failover implementation itself, since the API is harder to change later but the implementation should be straightforward to evolve over time. I do have one general (moderately nitpicky) request: Can you please remove all of the "just spacing" changes? There are many places where the spacing was changed due to stylistic preference but no text was changed. I liked it the way I wrote it though. :) Unless we adopt a style standard other than the minimal one we have (Sun Java standard style, 2 spaces for indentation, 80 char max per line) then I don't think such changes should be done without a good reason. It's also worth noting that Netbeans generated the Apache license headers automatically. All that said, I'm not trying to discourage anyone from fixing things, shortening lines over 80 chars, adding to the javadocs, or starting a discussion about style standards. :) flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13106> This should not be public. If it's public, people will cast from RpcClient to FailoverRpcClient to call it. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13107> Missing @Override annotation flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13108> Missing @Override annotation flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13109> Missing @Override annotation flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13118> client.isActive() does not imply that it's not closed. You have to clean up resources by closing it even if it's not active. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13119> safe to call over and over again flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13120> should probably happen outside the if block flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13110> This should not be public flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13121> This buildLock should not be necessary. What is static about it? flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java <https://reviews.apache.org/r/4380/#comment13073> This is not thread safe. Also not necessary, should be removed. We have to call close() on the transceiver to clean up resources. Actually, isConnected() is a confusing name. All it means is that an Avro handshake has been completed at some point. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13082> Maybe we should state that the failover is experimental and so the failover behavior policy is subject to change which is why it is not specified. Also note that this public static factory interface should not specify the implementation class that is returned. Not even in the javadocs. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13080> This should just be a getInstance() call with boolean failover argument = true flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13081> This should just be getInstance(Properties properties) with support for generating whatever is specified in the properties. User should not have to know there is a difference between the failover class and the composed class. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13078> I don't think we should expose this API publicly. We want to minimize the surface area of this thing - there should be one right way to do things in this API and nothing else should be public. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13079> This should not be exposed either. - Mike On 2012-03-19 20:43:56, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4380/ > ----------------------------------------------------------- > > (Updated 2012-03-19 20: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/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 9497a3d > flume-ng-sdk/src/test/java/org/apache/flume/api/TestFailoverRpcClient.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/4380/diff > > > Testing > ------- > > Unit tests added for the new functionality > > > Thanks, > > Hari > >
