> On 2012-03-28 06:03:54, Arvind Prabhakar wrote: > > Thanks for incorporating some of the previous feedback Hari. > > > > I did notice though that some of my feedback was not incorporated - which > > is OK as long as we discuss and agree to do so on the review. The best way > > to discuss this is by responding to individual comments with your feedback > > so that I know what to expect when you update the diffs. Without any > > comments from you, I expect that all of the feedback has been incorporated > > which is misleading. Conversely, if on every diff I have to do full review > > - then that poses a scalability challenge for the reviewer as you can see > > the number of diff increments that have gone into this issue alone. > > > > That said, I am putting marker comments on the items that were previously > > raised in the review and were not discussed or addressed. Please let me > > know your thoughts. > > > >
Interestingly, I had actually added a bunch of comments to your review. I will add them again here. Not sure why you cannot see them. Sorry for the inconvenience, I will try not to make the same mistake again. > On 2012-03-28 06:03:54, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java, > > line 180 > > <https://reviews.apache.org/r/4380/diff/21/?file=97183#file97183line180> > > > > Previous request. The only reason I did not add logs for these, is because they are not exactly error conditions, since the client is still ok, just one of the hosts just failed. I believe adding a logger.info might be ok. > On 2012-03-28 06:03:54, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java, > > lines 287-299 > > <https://reviews.apache.org/r/4380/diff/21/?file=97183#file97183line287> > > > > Previous request. I had added this in the previous review itself. Here it the reasoning. It is also mentioned in the code as a comment. Basically the logic is explained above(Mike's comments). Anyway here it is: We are currently connected to host number: i. i fails, we check all the hosts from i+1 to hosts.size()-1. If all fail, we check from host 0 to host i. Now, if none of these are available, throw exception. This is basically assuming that any host that fails might come back alive at some point in time. So we must go back and check every host and see if we can connect. - Hari ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4380/#review6473 ----------------------------------------------------------- On 2012-03-28 05:22:34, Hari Shreedharan wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4380/ > ----------------------------------------------------------- > > (Updated 2012-03-28 05:22:34) > > > 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 > >
