[
https://issues.apache.org/jira/browse/FLUME-962?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13240550#comment-13240550
]
[email protected] commented on FLUME-962:
-----------------------------------------------------
bq. On 2012-03-28 07:15:54, Mike Percy wrote:
bq. > Hari / Arvind,
bq. > I think we have made good progress with this patch. Also, the additional
comments really help with readability, thanks for adding them.
bq. >
bq. > However, the addition of the public configure() method on these objects
reduces maintainability and API coherence. I agree that these implementation
classes should read/handle their own configurations, however I think that would
be better achieved via a public static inner Builder similar to the one in
NettyAvroClient (the version that is currently checked in). We can make these
configure() methods private and disallow anyone from calling them except the
inner builder.
bq. >
bq. > The problem with public configure() methods is that they require us to
handle the case of in-flight reconfiguration. I also don't think the abstract
class helps here because people can cast to (AbstractRpcClient) and call
configure() if they want to because it's public. Although it should be frowned
upon, the language allows it.
bq. >
bq. > To instantiate and configure these objects in a way that enables
maintainable factory methods, while not increasing the surface area or
affecting the usability of the RpcClient API itself, I propose the following
API design:
bq. >
bq. > // inner builders in RpcClient implementation classes implement this
interface
bq. > public interface RpcClientBuilder {
bq. > public RpcClient build(Properties config);
bq. > }
bq. >
bq. > public NettyAvroRpcClient {
bq. > private NettyAvroRpcClient(...) { ... } // private constructor(s)
bq. > private configure(Properties config) { ... } // private configure()
method
bq. > public static class Builder implements RpcClientBuilder { ... } //
public builder or factory class
bq. > }
bq. >
bq. > // something similar to the above for FailoverRpcClient as well
bq. >
bq. > public RpcClientFactory {
bq. > private static enum ClientType {
bq. > SIMPLE(org.apache.flume.api.NettyAvroRpcClient.Builder.class),
bq. > FAILOVER(org.apache.flume.api.FailoverRpcClient.Builder.class),
bq. > OTHER(null);
bq. >
bq. > // inner enum refers to the builders, not the implementations
bq. > private Class<? extends RpcClientBuilder> clientClass;
bq. > // ...
bq. > }
bq. >
bq. > public RpcClient build(Properties config) {
bq. > // ... blah blah logic ...
bq. > return
ClientType.SIMPLE.getClientClass().newInstance().build(config); // for example
bq. > }
bq. > }
bq. >
bq. > The benefits of the above proposal are:
bq. > 1. Implementation classes are totally fungible from an interface
standpoint; casting doesn't buy you additional access to functionality (means
we can switch them later and guarantee binary compat)
bq. > 2. No need to maintain complicated state machines to determine when
configure() can be called and what to do in response (since it's private, we
can enforce that only the builder calls it)
bq. > 3. RpcClient interface doesn't change (stays minimal); you can only
append, check if it's active, and close it
bq. > 4. RpcClientBuilder interface combined with using Class<...> in the enum
allows us to (mostly) avoid reflection while also avoiding if-statements
bq. >
bq. > Do you guys feel me on this? :)
bq. >
bq. > If we can fix that, I think this patch is in decent enough shape to go
in (modulo a couple formatting issues).
bq. >
bq. > Regards,
bq. > Mike
bq. >
I personally will be happy with default constructors and public configure()
methods with work-arounds like raising exception on reconfiguration attempts
etc. But yes, as an API we can and should do better. In that regard, I do see
the merit in Mike's suggestion above.
Hari, what do you think?
- Arvind
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4380/#review6476
-----------------------------------------------------------
On 2012-03-28 07:25:01, 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-28 07:25:01)
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/AbstractRpcClient.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/RpcClient.java a601213
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, FLUME-962-rebased-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
