-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4380/#review6476
-----------------------------------------------------------
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
- Mike
On 2012-03-28 05:22:34, Hari Shreedharan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4380/
> -----------------------------------------------------------
>
> (Updated 2012-03-28 05:22:34)
>
>
> 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
>
>
