[
https://issues.apache.org/jira/browse/FLUME-1244?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13291362#comment-13291362
]
Arvind Prabhakar commented on FLUME-1244:
-----------------------------------------
Thanks for the review Mike! Great feedback as usual. See my comments below:
bq. In getClient(): if (!client.isActive()), then we should close() the
RpcClient before replacing the reference. That call cleans up the Netty
transceiver threads in the base client. Also, (!client.isActive()) should be an
"else if" condition, otherwise I think it's possible to create two clients in a
row very quickly in a failure case when the client starts as null and remains
inactive/dead after creation due to the host being down.
Done.
bq. In append(): I wonder how expensive it is to create a new HostInfo Iterator
on every append() operation. A higher level caching construct or iteration
abstraction might be more efficient. However the RoundRobinHostSelector
implementation is nice and elegant. This is just a note and I think we can
profile / optimize this later if needed.
Agree. Lets profile it and see. The thought did cross my mind as well.
bq. In close(): This method should call close() on all iterated RPC clients.
!isActive() does not imply that close() does not need to be called... there are
cases where a NettyRpcClient can be NEW or DEAD and have an active thread pool.
Also, close() is specified as an idempotent operation, but maybe that needs to
be clarified better in the RpcClient interface.
Done.
bq. In RandomOrderHostSelector.createHostIterator(): This line is a bit
disconcerting: indexOrder[indexList.size() - 1] = indexList.remove(pick);
One of my finer moments.
bq. It would would be good to store indexList.size() in a local var so we don't
have to look in the JLS to figure out the order of operations on that... ...
BTW, wouldn't the RHS be evaluated first? Or inside the square brackets would
happen first?
I refactored it to make it easy on the eyes. Apologies for having cryptic code.
bq. Nit: the test method names should start with a lowercase letter for Java
standard method naming
Another one of my finer moments :) Fixed it.
bq. Minor: in TestLbClientTenHostRoundRobinDistribution() and
TestLbClientTenHostRoundRobinDistributionBatch(): We do not have to rely on
distribution approximation for the asserts here. Since there is no randomness,
we can count the # of events and assert exactly as in some of the other
non-randomized tests.
I will let it be right now since at one time I was playing with different
values of the host and event size and there was some offsets due to rounding
that made this comparison better. But I agree - in the current state it does
not make much difference.
bq. That's all I have. Thanks for adding this important feature, all in all it
looks quite good.
Thanks again for a thorough review Mike.
> Implement a load-balancing RpcClient with round/robin and random distribution
> capabilties.
> ------------------------------------------------------------------------------------------
>
> Key: FLUME-1244
> URL: https://issues.apache.org/jira/browse/FLUME-1244
> Project: Flume
> Issue Type: Bug
> Reporter: Arvind Prabhakar
> Assignee: Arvind Prabhakar
> Fix For: v1.2.0
>
> Attachments: FLUME-1244-1.patch, FLUME-1244-2.patch
>
>
> We now have a load balancing sink processor implemented via FLUME-1198 which
> provides the ability to distribute load over a set of sinks via round/robin
> or random distribution scheme. A similar implementation should be available
> for the RpcClient within the client SDK.
--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators:
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira
