----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4047/#review5734 -----------------------------------------------------------
Thanks for the patch Mike. The changes are coming together nicely. I do have some feedback that follows. flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcCallback.java <https://reviews.apache.org/r/4047/#comment12482> Unused interface, should be removed. flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java <https://reviews.apache.org/r/4047/#comment12473> I suggest removing the FlumeRemoteException and FlumeTimeoutException in favor of EventDeliveryException as that is what the client is concerned with from user's perspective. Exposing these finer grain exceptions allow the creation of more sophisticated clients which runs contrary to the goal of keeping the client as simple to code as possible. Once we have concrete use-case from the field where exposing this semantic is necessary, we can easily introduce it at that time. Similarly we should remove the RpcResponse altogether. For blocking calls, the best way to communicate failure is via the exception and any more state insight that RpcResponse gives will add to the complexity of the client layer. flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java <https://reviews.apache.org/r/4047/#comment12474> This should be removed and the functionality should be managed within the client implementation via configuration. flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java <https://reviews.apache.org/r/4047/#comment12475> Same here as the previous comment. flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java <https://reviews.apache.org/r/4047/#comment12481> no such method #appendBatch(List, RpcCallback) flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java <https://reviews.apache.org/r/4047/#comment12471> nit: The regular idiom is to check for positive and not negative. So it would make sense for this method to be isActive() instead. flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClientBuilder.java <https://reviews.apache.org/r/4047/#comment12480> This should not be exposed but read via configuration. flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClientBuilder.java <https://reviews.apache.org/r/4047/#comment12476> These checks seem redundant since the builder will check them again anyway. Finally, please include some direct tests that exercise this API. - Arvind On 2012-03-08 11:14:53, Mike Percy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4047/ > ----------------------------------------------------------- > > (Updated 2012-03-08 11:14:53) > > > Review request for Flume. > > > Summary > ------- > > Seeking early feedback on some additional APIs to make integrating with Flume > 1.x easier. > > Added the following APIs: > - AvroClient: Friendly Java interface around the Avro API > - AvroClientBuilder: Builder class to allow easy extension of AvroClient > capabilities in the future (i.e. SSL) > - DefaultAvroClient: Implementation of the AvroClient interface > > Created this stuff in a flume-ng-sdk Maven submodule and moved the Event > interface to that submodule. flume-ng-core depends on flume-ng-sdk. > > I also modified AvroSink to use the AvroClient API instead of the bare > AvroSourceProtocol API directly. > > > This addresses bug FLUME-989. > https://issues.apache.org/jira/browse/FLUME-989 > > > Diffs > ----- > > flume-ng-clients/flume-ng-log4jappender/pom.xml 9dd31bd > > flume-ng-clients/flume-ng-log4jappender/src/main/java/org/apache/flume/clients/log4jappender/Log4jAppender.java > 97f2b9e > flume-ng-core/pom.xml fe6ce0b > flume-ng-core/src/main/avro/flume.avdl 40da3ef > flume-ng-core/src/main/java/org/apache/flume/Event.java a017705 > flume-ng-core/src/main/java/org/apache/flume/FlumeException.java eab5b3d > flume-ng-core/src/main/java/org/apache/flume/client/avro/AvroCLIClient.java > 195ba79 > flume-ng-core/src/main/java/org/apache/flume/event/EventBuilder.java > 5d8c3b3 > flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java e0c3b45 > flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 7386d06 > flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 467785f > flume-ng-sdk/pom.xml PRE-CREATION > flume-ng-sdk/src/main/avro/flume.avdl PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/Event.java PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/FlumeException.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/FlumeRemoteException.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/FlumeTimeoutException.java > PRE-CREATION > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/NettyAvroRpcClient.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcCallback.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java > PRE-CREATION > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClientBuilder.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcResponse.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/event/EventBuilder.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/event/SimpleEvent.java > PRE-CREATION > pom.xml d785762 > > Diff: https://reviews.apache.org/r/4047/diff > > > Testing > ------- > > > Thanks, > > Mike > >
