----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4380/#review6312 -----------------------------------------------------------
Hi, thanks for doing this work. I have the following suggestions on the config format: The properties format is not extensible. Right now it looks like this: hosts = host1 host2 host1 = hostname1:port1 host2 = hostname2:port2 How about something like: hosts = host1 host2 host.host1.endpoint = hostname1:port1 host.host2.endpoint = hostname2:port2 In this way, we namespace the host specifications, and also have the option of adding stuff like groups or priorities to them later if we want. Also, I have reservations about using the enum and reflection. My thoughts regarding that are below. flume-ng-sdk/src/main/java/org/apache/flume/api/ClientType.java <https://reviews.apache.org/r/4380/#comment13701> Does it really matter that it's backed by Netty? Maybe we can call this SIMPLE instead. flume-ng-sdk/src/main/java/org/apache/flume/api/ClientType.java <https://reviews.apache.org/r/4380/#comment13702> I think this should be declared final flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13729> This should be private. There is a public accessor method for the corresponding field. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13719> Shouldn't this start at -1 ? flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13711> We are indexing into this variable in this class. One should only use a LinkedList if there is only sequential iteration happening. Since we are directly indexing into this variable we should be using an ArrayList instead. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13718> Is this max total tries or max retries? There's a difference of one between them. Also it might make sense to name this property MaxTries. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13714> Why assign to client member variable when getClient() already assigned to it? flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13708> Should only set this within a synchronized block if it is guarded by (this). A private synchronized setter would work. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13704> We should not throw vanilla FlumeException. It should be EventDeliveryException only (per the RpcClient interface). flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13703> Can be dangerous / misleading to name a local variable with the same name as a member variable. Consider renaming to curClient or something. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13705> Should throw EventDeliveryException here. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13706> We should not throw FlumeException from this method, per the RpcClient interface. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13715> Masks member variable. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13707> Throw EventDeliveryException flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13716> while (tries < maxTries) Consider using a for() loop instead of a while(). flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13717> if (tries == maxTries) flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13710> masks member variable flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13709> Why not while() { ... } ? Generally, can the logic here be simplified? It took me a long time to trace through the code. It seems complex and is using several variables. Maybe base the logic on a variable called numTries, and loop while (numTries < hosts.length - 1). It's hard to follow if both count and limit are changed inside the loop like this. Also, I think there is an off-by-one error here because if lastCheckedHost starts @ 0, and hosts.length == 3, then you will only call getInstance() twice before bailing out. But if lastCheckedHost == 2 when beginning, then getInstance() will be called 3 times. One problem is that the case where we are trying for the first time is maybe not handled explicitly differently than the case where we are doing a retry. Might make sense to separate that logic out a little more. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java <https://reviews.apache.org/r/4380/#comment13728> Might be a good idea to remove this method. Just use the Properties one. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13724> Let's not document the default batchSize. They should use getBatchSize() to detect it. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java <https://reviews.apache.org/r/4380/#comment13727> I'm not excited about using reflection here. We lose traceability in IDEs and compile-time type checking. Also, the enum is not even used directly it's just basically reflected too. So what's the benefit. - Mike 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 > >
