> On 2012-03-08 18:29:17, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcCallback.java, > > line 18 > > <https://reviews.apache.org/r/4047/diff/2/?file=89096#file89096line18> > > > > Unused interface, should be removed.
Done > On 2012-03-08 18:29:17, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java, line > > 28 > > <https://reviews.apache.org/r/4047/diff/2/?file=89097#file89097line28> > > > > 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. Done. Note that EventDeliveryException is checked and we should consider making it inherit from FlumeException for consistency. But I didn't try to make that change here since it would not be backwards compatible. > On 2012-03-08 18:29:17, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java, line > > 52 > > <https://reviews.apache.org/r/4047/diff/2/?file=89097#file89097line52> > > > > This should be removed and the functionality should be managed within > > the client implementation via configuration. I simply removed the functionality from the interface and the default timeouts are used only for now. > On 2012-03-08 18:29:17, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java, line > > 80 > > <https://reviews.apache.org/r/4047/diff/2/?file=89097#file89097line80> > > > > Same here as the previous comment. > > Done. > On 2012-03-08 18:29:17, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java, line > > 85 > > <https://reviews.apache.org/r/4047/diff/2/?file=89097#file89097line85> > > > > no such method #appendBatch(List, RpcCallback) Thanks, fixed! > On 2012-03-08 18:29:17, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClient.java, line > > 106 > > <https://reviews.apache.org/r/4047/diff/2/?file=89097#file89097line106> > > > > nit: The regular idiom is to check for positive and not negative. So it > > would make sense for this method to be isActive() instead. I just removed it, as mentioned above. > On 2012-03-08 18:29:17, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClientBuilder.java, > > line 90 > > <https://reviews.apache.org/r/4047/diff/2/?file=89098#file89098line90> > > > > This should not be exposed but read via configuration. Hid this functionality for now. > On 2012-03-08 18:29:17, Arvind Prabhakar wrote: > > flume-ng-sdk/src/main/java/org/apache/flume/api/client/RpcClientBuilder.java, > > lines 121-123 > > <https://reviews.apache.org/r/4047/diff/2/?file=89098#file89098line121> > > > > These checks seem redundant since the builder will check them again > > anyway. Done. On 2012-03-08 18:29:17, Mike Percy wrote: > > Finally, please include some direct tests that exercise this API. Added a bunch of tests that really put the API through its paces. - Mike ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/4047/#review5734 ----------------------------------------------------------- On 2012-03-09 11:14:13, Mike Percy wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/4047/ > ----------------------------------------------------------- > > (Updated 2012-03-09 11:14:13) > > > 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 5278fc0 > flume-ng-core/src/main/java/org/apache/flume/EventDeliveryException.java > 1413223 > 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/main/java/org/apache/flume/source/AvroSource.java a3f6640 > flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 467785f > flume-ng-core/src/test/java/org/apache/flume/util/TestEventBuilder.java > 7930607 > 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/EventDeliveryException.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/api/NettyAvroRpcClient.java > PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClient.java PRE-CREATION > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.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 > > flume-ng-sdk/src/test/java/org/apache/flume/api/NettyAvroRpcTestHelpers.java > PRE-CREATION > flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java > PRE-CREATION > flume-ng-sdk/src/test/java/org/apache/flume/api/TestRpcClientFactory.java > PRE-CREATION > flume-ng-sdk/src/test/java/org/apache/flume/event/TestEventBuilder.java > PRE-CREATION > pom.xml d785762 > > Diff: https://reviews.apache.org/r/4047/diff > > > Testing > ------- > > > Thanks, > > Mike > >
