> On 2012-03-28 07:15:54, Mike Percy wrote:
> > Hari / Arvind,
> > I think we have made good progress with this patch. Also, the additional
> > comments really help with readability, thanks for adding them.
> >
> > 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.
> >
> > 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.
> >
> > 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:
> >
> > // inner builders in RpcClient implementation classes implement this
> > interface
> > public interface RpcClientBuilder {
> > public RpcClient build(Properties config);
> > }
> >
> > public NettyAvroRpcClient {
> > private NettyAvroRpcClient(...) { ... } // private constructor(s)
> > private configure(Properties config) { ... } // private configure() method
> > public static class Builder implements RpcClientBuilder { ... } // public
> > builder or factory class
> > }
> >
> > // something similar to the above for FailoverRpcClient as well
> >
> > public RpcClientFactory {
> > private static enum ClientType {
> > SIMPLE(org.apache.flume.api.NettyAvroRpcClient.Builder.class),
> > FAILOVER(org.apache.flume.api.FailoverRpcClient.Builder.class),
> > OTHER(null);
> >
> > // inner enum refers to the builders, not the implementations
> > private Class<? extends RpcClientBuilder> clientClass;
> > // ...
> > }
> >
> > public RpcClient build(Properties config) {
> > // ... blah blah logic ...
> > return ClientType.SIMPLE.getClientClass().newInstance().build(config);
> > // for example
> > }
> > }
> >
> > The benefits of the above proposal are:
> > 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)
> > 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)
> > 3. RpcClient interface doesn't change (stays minimal); you can only append,
> > check if it's active, and close it
> > 4. RpcClientBuilder interface combined with using Class<...> in the enum
> > allows us to (mostly) avoid reflection while also avoiding if-statements
> >
> > Do you guys feel me on this? :)
> >
> > If we can fix that, I think this patch is in decent enough shape to go in
> > (modulo a couple formatting issues).
> >
> > Regards,
> > Mike
> >
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:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4380/
> -----------------------------------------------------------
>
> (Updated 2012-03-28 07:25:01)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> 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.
>
> 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.
>
>
> This addresses bug FLUME-962.
> https://issues.apache.org/jira/browse/FLUME-962
>
>
> Diffs
> -----
>
> flume-ng-sdk/src/main/java/org/apache/flume/api/AbstractRpcClient.java
> PRE-CREATION
> flume-ng-sdk/src/main/java/org/apache/flume/api/FailoverRpcClient.java
> PRE-CREATION
> flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java
> 965b2ff
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClient.java a601213
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java
> 351b5b1
> flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 93bfee9
> flume-ng-sdk/src/test/java/org/apache/flume/api/TestFailoverRpcClient.java
> PRE-CREATION
> flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java
> a33e9c8
> flume-ng-sdk/src/test/java/org/apache/flume/api/TestRpcClientFactory.java
> 0c94231
>
> Diff: https://reviews.apache.org/r/4380/diff
>
>
> Testing
> -------
>
> Unit tests added for the new functionality
>
>
> Thanks,
>
> Hari
>
>
