[
https://issues.apache.org/jira/browse/FLUME-962?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13237364#comment-13237364
]
[email protected] commented on FLUME-962:
-----------------------------------------------------
-----------------------------------------------------------
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:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/4380/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2012-03-22 03:43:56)
bq.
bq.
bq. Review request for Flume.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. 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.
bq.
bq. 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.
bq.
bq.
bq. This addresses bug FLUME-962.
bq. https://issues.apache.org/jira/browse/FLUME-962
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. flume-ng-sdk/src/main/java/org/apache/flume/api/ClientType.java
PRE-CREATION
bq. flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
PRE-CREATION
bq. flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java
965b2ff
bq. flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java
351b5b1
bq. flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java
93bfee9
bq.
flume-ng-sdk/src/test/java/org/apache/flume/api/TestFailoverRpcClient.java
PRE-CREATION
bq.
flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java
a33e9c8
bq.
flume-ng-sdk/src/test/java/org/apache/flume/api/TestRpcClientFactory.java
0c94231
bq.
bq. Diff: https://reviews.apache.org/r/4380/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. Unit tests added for the new functionality
bq.
bq.
bq. Thanks,
bq.
bq. Hari
bq.
bq.
> Failover capability for Client SDK
> ----------------------------------
>
> Key: FLUME-962
> URL: https://issues.apache.org/jira/browse/FLUME-962
> Project: Flume
> Issue Type: Sub-task
> Affects Versions: v1.0.0
> Reporter: Kathleen Ting
> Assignee: Hari Shreedharan
> Fix For: v1.2.0
>
> Attachments: FLUME-962-2.patch, FLUME-962-2.patch, FLUME-962-3.patch,
> FLUME-962-3.patch, FLUME-962-4.patch, FLUME-962-5.patch, FLUME-962-6.patch,
> FLUME-962-rebased-1.patch
>
>
> Need a client SDK for Flume that will allow clients to be able to failover
> from one source to another in case the first agent is not available. This
> will help in keeping client implementations developed outside of the project
> decoupled from internal details of HA implementation within Flume.
--
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
