[
https://issues.apache.org/jira/browse/FLUME-1244?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13291300#comment-13291300
]
Mike Percy commented on FLUME-1244:
-----------------------------------
Nice work. This functionality is sorely needed!
I have the following code review feedback:
LoadBalancingRpcClient.java
===========================
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.
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.
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.
In RandomOrderHostSelector.createHostIterator(): This line is a bit
disconcerting: indexOrder[indexList.size() - 1] = indexList.remove(pick);
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?
TestLoadBalancingRpcClient.java
===============================
Nit: the test method names should start with a lowercase letter for Java
standard method naming
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.
That's all I have. Thanks for adding this important feature, all in all it
looks quite good.
> 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
>
>
> 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
