You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Mike Percy <mp...@gmail.com> on 2012/03/08 12:14:53 UTC
Re: Review Request: FLUME-989 Client SDK API
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4047/
-----------------------------------------------------------
(Updated 2012-03-08 11:14:53.728421)
Review request for Flume.
Changes
-------
Now applies to merged trunk.
Summary (updated)
-------
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
Re: Review Request: FLUME-989 Client SDK API
Posted by Mike Percy <mp...@gmail.com>.
> 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
>
>
Re: Review Request: FLUME-989 Client SDK API
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-989 Client SDK API
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4047/#review5841
-----------------------------------------------------------
Thanks for the patch Mike. I have one suggestion though:
Please modify the default appendBatch() method in the NettyAvroRpcClient() implementation so that it does not throw an IllegalArgumentException if the passed in collection contains more events than the allowed batch size. Instead it should loop over the given collection in chunks of batch sizes until all of it has been drained to the downstream end point.
Reason for this ask is that the builder can potentially use the built in default value of batch size that is not visible to the client directly.
- Arvind
On 2012-03-09 23:44:00, Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4047/
> -----------------------------------------------------------
>
> (Updated 2012-03-09 23:44:00)
>
>
> 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/RpcTestUtils.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
>
>
Re: Review Request: FLUME-989 Client SDK API
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4047/#review5866
-----------------------------------------------------------
Ship it!
+1. Changes look good Mike. Please attach the patch to the Jira.
- Arvind
On 2012-03-12 23:33:32, Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4047/
> -----------------------------------------------------------
>
> (Updated 2012-03-12 23:33:32)
>
>
> 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-channels/flume-file-channel/pom.xml ee2d20f
> flume-ng-channels/flume-jdbc-channel/pom.xml 51a7694
> 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/source/TestAvroSource.java c5c3f2f
> flume-ng-core/src/test/java/org/apache/flume/util/TestEventBuilder.java 7930607
> flume-ng-legacy-sources/flume-avro-source/pom.xml 2d53403
> flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java 65d9142
> flume-ng-legacy-sources/flume-avro-source/src/test/java/org/apache/flume/source/avroLegacy/TestLegacyAvroSource.java 6e3eb53
> flume-ng-legacy-sources/flume-thrift-source/pom.xml 8256f0f
> flume-ng-node/pom.xml b9b062e
> 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/RpcTestUtils.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
> flume-ng-sinks/flume-hdfs-sink/pom.xml f27851e
> flume-ng-sinks/flume-irc-sink/pom.xml c885f35
> pom.xml d785762
>
> Diff: https://reviews.apache.org/r/4047/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Mike
>
>
Re: Review Request: FLUME-989 Client SDK API
Posted by Mike Percy <mp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4047/
-----------------------------------------------------------
(Updated 2012-03-12 23:33:32.264619)
Review request for Flume.
Changes
-------
Patch updated to allow multiple RPC calls from one batch call if getBatchSize() is not respected. Also added some API docs indicating the behavior and implications of this. I also found a maven configuration param to make the Java generated from Avro RPC use String instead of CharSequence as the String type, which will result in less object copying in our code. I included that, since it primarily affects the RPC client implementation. Finally, one notable API change is that I have declared the NettyAvroRpcClient.Builder class to be protected instead of public. This is to enforce usage of the factory methods in RpcClientFactory, except for in unit tests.
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 (updated)
-----
flume-ng-channels/flume-file-channel/pom.xml ee2d20f
flume-ng-channels/flume-jdbc-channel/pom.xml 51a7694
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/source/TestAvroSource.java c5c3f2f
flume-ng-core/src/test/java/org/apache/flume/util/TestEventBuilder.java 7930607
flume-ng-legacy-sources/flume-avro-source/pom.xml 2d53403
flume-ng-legacy-sources/flume-avro-source/src/main/java/org/apache/flume/source/avroLegacy/AvroLegacySource.java 65d9142
flume-ng-legacy-sources/flume-avro-source/src/test/java/org/apache/flume/source/avroLegacy/TestLegacyAvroSource.java 6e3eb53
flume-ng-legacy-sources/flume-thrift-source/pom.xml 8256f0f
flume-ng-node/pom.xml b9b062e
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/RpcTestUtils.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
flume-ng-sinks/flume-hdfs-sink/pom.xml f27851e
flume-ng-sinks/flume-irc-sink/pom.xml c885f35
pom.xml d785762
Diff: https://reviews.apache.org/r/4047/diff
Testing
-------
Thanks,
Mike
Re: Review Request: FLUME-989 Client SDK API
Posted by Mike Percy <mp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4047/
-----------------------------------------------------------
(Updated 2012-03-09 23:44:00.840039)
Review request for Flume.
Changes
-------
Patch updated. I brought back isAlive() but it basically just guarantees that we haven't seen a major error happen yet.
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 (updated)
-----
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/RpcTestUtils.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
Re: Review Request: FLUME-989 Client SDK API
Posted by Mike Percy <mp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4047/#review5798
-----------------------------------------------------------
After sleeping on it, I am going to put isAlive() back. I don't like the semantics of having to catch exceptions and save state for the next call. So please wait for me to update the patch before reviewing.
- Mike
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
>
>
Re: Review Request: FLUME-989 Client SDK API
Posted by Mike Percy <mp...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4047/
-----------------------------------------------------------
(Updated 2012-03-09 11:14:13.656079)
Review request for Flume.
Changes
-------
I applied all of the feedback as well as went a bit farther. The semantics of isAlive() were kind of weird so I just removed it completely and now if you get an EventDeliveryException you must close the client and construct a new one. It's a bit heavy but it's a contract we can build on as we evolve the system and APIs.
I also found an implementation bug in AvroSink where it will never throw an EventDeliveryException. So I fixed that to be consistent with the behavior we have been discussing w.r.t. Javadocs lately. Now it only uses BACKOFF for an empty feeder channel.
I changed the builder to be an inner class and I left the factory methods as a factory. I think that gives us both flexibility in the future w/ the builder and simplicity for now w/ the factory.
I wrote a whole bunch of unit tests as well. They uncovered some issues which I have addressed.
This patch applies to the latest trunk.
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 (updated)
-----
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