-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9427/
-----------------------------------------------------------

Review request for Flume.
Description
-------

Enhance NettyAvroRpcClient and the use of NettyServer to optionally use compression

Description: This JIRA will update AvroSource and NettyAvroRpcClient to optionally use ZlibEncoder and ZlibDecoder to compress communications between themselves.

Patch Comment: Added compression options for both client and server.
Note compress has be turn on both client and server for it to work.
Client can have compression with out compression if compression level is set to 0
This addresses bug FLUME-1915.
    https://issues.apache.org/jira/browse/FLUME-1915
Diffs
-----

  flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java dc18c5d
  flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 4bf36e6
  flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java cf9724c
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java 1e642d8
  flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 5042d11
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 99ae010

Diff: https://reviews.apache.org/r/9427/diff/
Testing
-------

Yes
Thanks,

Ted Malaska

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9427/#review16982
-----------------------------------------------------------
Hey Ted,
Since it doesn't look like we really have good hooks for specifying headers or dynamically changing the Netty pipeline, the implementation looks good. A few points of feedback:
1. Let's plumb the configuration parameters from the client down to the AbstractRpcSink.configure() method to enable compression at the Avro Sink level
2. How about a unit test for the Avro Sink with compression talking to an Avro Source with compression?
3. It would be great to update the user docs for the AvroSource & AvroSink @ flume-ng-doc/sphinx/FlumeUserGuide.rst and the RpcClient @ flume-ng-doc/sphinx/FlumeDeveloperGuide.rst
4. In order to enable extension in the future (such as support for snappy or some other compression codec), how about we change the config param compression-enabled to compression-type = none | zlib

Does that sound alright?

Regards,
Mike

- Mike Percy
On Feb. 13, 2013, 5:02 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9427/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2013, 5:02 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Enhance NettyAvroRpcClient and the use of NettyServer to optionally use compression
>
> Description: This JIRA will update AvroSource and NettyAvroRpcClient to optionally use ZlibEncoder and ZlibDecoder to compress communications between themselves.
>
> Patch Comment: Added compression options for both client and server.
> Note compress has be turn on both client and server for it to work.
> Client can have compression with out compression if compression level is set to 0
>
>
> This addresses bug FLUME-1915.
>     https://issues.apache.org/jira/browse/FLUME-1915
>
>
> Diffs
> -----
>
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java dc18c5d
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 4bf36e6
>   flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java cf9724c
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java 1e642d8
>   flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 5042d11
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 99ae010
>
> Diff: https://reviews.apache.org/r/9427/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Ted Malaska
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9427/#review16959
-----------------------------------------------------------
My only concern is that a client who doesn't know about compression cannot talk to a server with compression enabled. Is there any way to detect that the client is not compressed and handle it accordingly at the server?

That way, all servers can enable compression and maintain backcompat with existing clients.

- Mike Percy
On Feb. 13, 2013, 5:02 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9427/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2013, 5:02 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Enhance NettyAvroRpcClient and the use of NettyServer to optionally use compression
>
> Description: This JIRA will update AvroSource and NettyAvroRpcClient to optionally use ZlibEncoder and ZlibDecoder to compress communications between themselves.
>
> Patch Comment: Added compression options for both client and server.
> Note compress has be turn on both client and server for it to work.
> Client can have compression with out compression if compression level is set to 0
>
>
> This addresses bug FLUME-1915.
>     https://issues.apache.org/jira/browse/FLUME-1915
>
>
> Diffs
> -----
>
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java dc18c5d
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 4bf36e6
>   flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java cf9724c
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java 1e642d8
>   flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 5042d11
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 99ae010
>
> Diff: https://reviews.apache.org/r/9427/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Ted Malaska
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9427/#review16926
-----------------------------------------------------------

Ship it!
+1 looks good to me! Ran into some JUnit problem in IntelliJ but I plan on committing this as soon as I can check a couple things from an IDE tomorrow.

- Mike Percy
On Feb. 13, 2013, 5:02 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9427/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2013, 5:02 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Enhance NettyAvroRpcClient and the use of NettyServer to optionally use compression
>
> Description: This JIRA will update AvroSource and NettyAvroRpcClient to optionally use ZlibEncoder and ZlibDecoder to compress communications between themselves.
>
> Patch Comment: Added compression options for both client and server.
> Note compress has be turn on both client and server for it to work.
> Client can have compression with out compression if compression level is set to 0
>
>
> This addresses bug FLUME-1915.
>     https://issues.apache.org/jira/browse/FLUME-1915
>
>
> Diffs
> -----
>
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java dc18c5d
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 4bf36e6
>   flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java cf9724c
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java 1e642d8
>   flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 5042d11
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 99ae010
>
> Diff: https://reviews.apache.org/r/9427/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Ted Malaska
>
>

> On Feb. 22, 2013, 8:11 a.m., Mike Percy wrote:
> > +1 looks good to me! Ran into some JUnit problem in IntelliJ but I plan on committing this as soon as I can check a couple things from an IDE tomorrow.

Thanks Mike.
- Ted
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9427/#review16926
-----------------------------------------------------------
On Feb. 13, 2013, 5:02 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9427/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2013, 5:02 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Enhance NettyAvroRpcClient and the use of NettyServer to optionally use compression
>
> Description: This JIRA will update AvroSource and NettyAvroRpcClient to optionally use ZlibEncoder and ZlibDecoder to compress communications between themselves.
>
> Patch Comment: Added compression options for both client and server.
> Note compress has be turn on both client and server for it to work.
> Client can have compression with out compression if compression level is set to 0
>
>
> This addresses bug FLUME-1915.
>     https://issues.apache.org/jira/browse/FLUME-1915
>
>
> Diffs
> -----
>
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java dc18c5d
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 4bf36e6
>   flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java cf9724c
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java 1e642d8
>   flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 5042d11
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 99ae010
>
> Diff: https://reviews.apache.org/r/9427/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Ted Malaska
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9427/#review16985
-----------------------------------------------------------

flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java
<https://reviews.apache.org/r/9427/#comment35985>

    Should be "true".equalsIgnoresCase(enableCompressionStr).
- Brock Noland
On Feb. 13, 2013, 5:02 p.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9427/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2013, 5:02 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Enhance NettyAvroRpcClient and the use of NettyServer to optionally use compression
>
> Description: This JIRA will update AvroSource and NettyAvroRpcClient to optionally use ZlibEncoder and ZlibDecoder to compress communications between themselves.
>
> Patch Comment: Added compression options for both client and server.
> Note compress has be turn on both client and server for it to work.
> Client can have compression with out compression if compression level is set to 0
>
>
> This addresses bug FLUME-1915.
>     https://issues.apache.org/jira/browse/FLUME-1915
>
>
> Diffs
> -----
>
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java dc18c5d
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 4bf36e6
>   flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java cf9724c
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java 1e642d8
>   flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 5042d11
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 99ae010
>
> Diff: https://reviews.apache.org/r/9427/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Ted Malaska
>
>

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9427/
-----------------------------------------------------------

(Updated Feb. 27, 2013, 4:34 a.m.)
Review request for Flume.
Changes
-------

Addresses all review comments
Description
-------

Enhance NettyAvroRpcClient and the use of NettyServer to optionally use compression

Description: This JIRA will update AvroSource and NettyAvroRpcClient to optionally use ZlibEncoder and ZlibDecoder to compress communications between themselves.

Patch Comment: Added compression options for both client and server.
Note compress has be turn on both client and server for it to work.
Client can have compression with out compression if compression level is set to 0
This addresses bug FLUME-1915.
    https://issues.apache.org/jira/browse/FLUME-1915
Diffs (updated)
-----

  flume-ng-core/src/main/java/org/apache/flume/sink/AbstractRpcSink.java 52bd49b
  flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java dc18c5d
  flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java b9e59ef
  flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 4bf36e6
  flume-ng-doc/sphinx/FlumeUserGuide.rst 8eb3734
  flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java cf9724c
  flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java 1e642d8
  flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 5042d11
  flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 99ae010

Diff: https://reviews.apache.org/r/9427/diff/
Testing
-------

Yes
Thanks,

Ted Malaska

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9427/#review17583
-----------------------------------------------------------

Ship it!
+1, lgtm, I am going to commit this. I have one request for a minor follow-up patch inline.
flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/9427/#comment37291>

    Can we file a follow-up JIRA to make this only active when compressionType == none or null/empty? A validation / usability nit.
- Mike Percy
On Feb. 27, 2013, 4:34 a.m., Ted Malaska wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9427/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2013, 4:34 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Enhance NettyAvroRpcClient and the use of NettyServer to optionally use compression
>
> Description: This JIRA will update AvroSource and NettyAvroRpcClient to optionally use ZlibEncoder and ZlibDecoder to compress communications between themselves.
>
> Patch Comment: Added compression options for both client and server.
> Note compress has be turn on both client and server for it to work.
> Client can have compression with out compression if compression level is set to 0
>
>
> This addresses bug FLUME-1915.
>     https://issues.apache.org/jira/browse/FLUME-1915
>
>
> Diffs
> -----
>
>   flume-ng-core/src/main/java/org/apache/flume/sink/AbstractRpcSink.java 52bd49b
>   flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java dc18c5d
>   flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java b9e59ef
>   flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java 4bf36e6
>   flume-ng-doc/sphinx/FlumeUserGuide.rst 8eb3734
>   flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java cf9724c
>   flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java 1e642d8
>   flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java 5042d11
>   flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 99ae010
>
> Diff: https://reviews.apache.org/r/9427/diff/
>
>
> Testing
> -------
>
> Yes
>
>
> Thanks,
>
> Ted Malaska
>
>