[
https://issues.apache.org/jira/browse/FLUME-962?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13233040#comment-13233040
]
[email protected] commented on FLUME-962:
-----------------------------------------------------
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4380/#review6089
-----------------------------------------------------------
Thanks for the patch!
I've added some feedback on the API inline in the review.
I mostly just have comments on the API, not on the failover implementation
itself, since the API is harder to change later but the implementation should
be straightforward to evolve over time.
I do have one general (moderately nitpicky) request: Can you please remove all
of the "just spacing" changes? There are many places where the spacing was
changed due to stylistic preference but no text was changed. I liked it the way
I wrote it though. :) Unless we adopt a style standard other than the minimal
one we have (Sun Java standard style, 2 spaces for indentation, 80 char max per
line) then I don't think such changes should be done without a good reason.
It's also worth noting that Netbeans generated the Apache license headers
automatically. All that said, I'm not trying to discourage anyone from fixing
things, shortening lines over 80 chars, adding to the javadocs, or starting a
discussion about style standards. :)
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13106>
This should not be public. If it's public, people will cast from RpcClient
to FailoverRpcClient to call it.
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13107>
Missing @Override annotation
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13108>
Missing @Override annotation
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13109>
Missing @Override annotation
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13118>
client.isActive() does not imply that it's not closed. You have to clean up
resources by closing it even if it's not active.
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13119>
safe to call over and over again
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13120>
should probably happen outside the if block
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13110>
This should not be public
flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
<https://reviews.apache.org/r/4380/#comment13121>
This buildLock should not be necessary. What is static about it?
flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java
<https://reviews.apache.org/r/4380/#comment13073>
This is not thread safe. Also not necessary, should be removed. We have to
call close() on the transceiver to clean up resources.
Actually, isConnected() is a confusing name. All it means is that an Avro
handshake has been completed at some point.
flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java
<https://reviews.apache.org/r/4380/#comment13082>
Maybe we should state that the failover is experimental and so the failover
behavior policy is subject to change which is why it is not specified.
Also note that this public static factory interface should not specify the
implementation class that is returned. Not even in the javadocs.
flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java
<https://reviews.apache.org/r/4380/#comment13080>
This should just be a getInstance() call with boolean failover argument =
true
flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java
<https://reviews.apache.org/r/4380/#comment13081>
This should just be getInstance(Properties properties) with support for
generating whatever is specified in the properties. User should not have to
know there is a difference between the failover class and the composed class.
flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java
<https://reviews.apache.org/r/4380/#comment13078>
I don't think we should expose this API publicly. We want to minimize the
surface area of this thing - there should be one right way to do things in this
API and nothing else should be public.
flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java
<https://reviews.apache.org/r/4380/#comment13079>
This should not be exposed either.
- Mike
On 2012-03-19 20: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-19 20: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/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
9497a3d
bq.
flume-ng-sdk/src/test/java/org/apache/flume/api/TestFailoverRpcClient.java
PRE-CREATION
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
> 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
>
>
> 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
