You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Mubarak Seyed <se...@apple.com> on 2012/07/01 01:06:28 UTC
Re: Review Request: FLUME-1316: AvroSink should be configurable for
connect-timeout and request-timeout
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5683/#review8773
-----------------------------------------------------------
lgtm. Few comments.
- Are we trying to use these two properties (connect-timeout and request-timeout) to both AvroSink and AvroRpc (using Properties to RpcClient)? Both uses RpcClientFactory with properties. I think AvroSink client.type property should honor only DEFAULT (and ignore DEFAULT_FAILOVER and DEFAULT_LOADBALANCE)
flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java
<https://reviews.apache.org/r/5683/#comment18546>
I think we need to add connect-timeout and request-timeout javadoc here? values in milliseconds
flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java
<https://reviews.apache.org/r/5683/#comment18545>
flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java
<https://reviews.apache.org/r/5683/#comment18547>
counter for failure here? will be used in metrics (for rollbacks and exceptions)
flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java
<https://reviews.apache.org/r/5683/#comment18548>
single space needed before "ms"
flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java
<https://reviews.apache.org/r/5683/#comment18549>
single space needed before "ms"
flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java
<https://reviews.apache.org/r/5683/#comment18544>
If batchSize is null, can't we set it to 1 or
RpcClientConfigurationConstants.DEFAULT_BATCH_SIZE. I think hostName and port are mandatory for client connection.
- Mubarak Seyed
On June 30, 2012, 12:36 a.m., Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5683/
> -----------------------------------------------------------
>
> (Updated June 30, 2012, 12:36 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Patch to add support for configurable connect and request timeout to Avro Sink. Also refactors some of the RpcClient libs to reduce the # of codepaths.
>
>
> This addresses bug FLUME-1316.
> https://issues.apache.org/jira/browse/FLUME-1316
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5c6d0e3
> flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 3765924
> flume-ng-doc/sphinx/FlumeUserGuide.rst 94f951f
> flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java 606a4bd
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java e19b093
> flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java e4f23a6
> flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 77bf331
>
> Diff: https://reviews.apache.org/r/5683/diff/
>
>
> Testing
> -------
>
> Added a unit test for the new params. Existing unit tests pass.
>
>
> Thanks,
>
> Mike Percy
>
>
Re: Review Request: FLUME-1316: AvroSink should be configurable for
connect-timeout and request-timeout
Posted by Mubarak Seyed <se...@apple.com>.
> On June 30, 2012, 11:06 p.m., Mubarak Seyed wrote:
> > lgtm. Few comments.
> >
> > - Are we trying to use these two properties (connect-timeout and request-timeout) to both AvroSink and AvroRpc (using Properties to RpcClient)? Both uses RpcClientFactory with properties. I think AvroSink client.type property should honor only DEFAULT (and ignore DEFAULT_FAILOVER and DEFAULT_LOADBALANCE)
> >
Please ignore my previous comment. AvroSink builds Properties and it doesn't set the client.type hence it defaults to DEFAULT. AvroRpc can specify timeout values.
- Mubarak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5683/#review8773
-----------------------------------------------------------
On June 30, 2012, 12:36 a.m., Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5683/
> -----------------------------------------------------------
>
> (Updated June 30, 2012, 12:36 a.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Patch to add support for configurable connect and request timeout to Avro Sink. Also refactors some of the RpcClient libs to reduce the # of codepaths.
>
>
> This addresses bug FLUME-1316.
> https://issues.apache.org/jira/browse/FLUME-1316
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5c6d0e3
> flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 3765924
> flume-ng-doc/sphinx/FlumeUserGuide.rst 94f951f
> flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java 606a4bd
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java e19b093
> flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java e4f23a6
> flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 77bf331
>
> Diff: https://reviews.apache.org/r/5683/diff/
>
>
> Testing
> -------
>
> Added a unit test for the new params. Existing unit tests pass.
>
>
> Thanks,
>
> Mike Percy
>
>
Re: Review Request: FLUME-1316: AvroSink should be configurable for
connect-timeout and request-timeout
Posted by Mike Percy <mp...@gmail.com>.
> On June 30, 2012, 11:06 p.m., Mubarak Seyed wrote:
> > flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java, line 99
> > <https://reviews.apache.org/r/5683/diff/1/?file=117624#file117624line99>
> >
> > I think we need to add connect-timeout and request-timeout javadoc here? values in milliseconds
Added these Javadocs
> On June 30, 2012, 11:06 p.m., Mubarak Seyed wrote:
> > flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java, line 297
> > <https://reviews.apache.org/r/5683/diff/1/?file=117624#file117624line297>
> >
> > counter for failure here? will be used in metrics (for rollbacks and exceptions)
Added a counter
> On June 30, 2012, 11:06 p.m., Mubarak Seyed wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java, line 188
> > <https://reviews.apache.org/r/5683/diff/1/?file=117627#file117627line188>
> >
> > single space needed before "ms"
This is a pretty nitpicky comment, but I added the space anyway for ease of using awk/perl to parse the logs ;)
> On June 30, 2012, 11:06 p.m., Mubarak Seyed wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java, line 161
> > <https://reviews.apache.org/r/5683/diff/1/?file=117628#file117628line161>
> >
> > If batchSize is null, can't we set it to 1 or
> > RpcClientConfigurationConstants.DEFAULT_BATCH_SIZE. I think hostName and port are mandatory for client connection.
This is the 3-arg version of the factory method. If developers don't want to specify batch size, there is the 2-arg factory method as well as the Properties-driven factory method available for them to use.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5683/#review8773
-----------------------------------------------------------
On July 3, 2012, 7:35 p.m., Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5683/
> -----------------------------------------------------------
>
> (Updated July 3, 2012, 7:35 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Patch to add support for configurable connect and request timeout to Avro Sink. Also refactors some of the RpcClient libs to reduce the # of codepaths.
>
>
> This addresses bug FLUME-1316.
> https://issues.apache.org/jira/browse/FLUME-1316
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5c6d0e3
> flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 3765924
> flume-ng-doc/sphinx/FlumeUserGuide.rst 94f951f
> flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java 606a4bd
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java e304689
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java e19b093
> flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java e4f23a6
> flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 77bf331
>
> Diff: https://reviews.apache.org/r/5683/diff/
>
>
> Testing
> -------
>
> Added a unit test for the new params. Existing unit tests pass.
>
>
> Thanks,
>
> Mike Percy
>
>
Re: Review Request: FLUME-1316: AvroSink should be configurable for
connect-timeout and request-timeout
Posted by Mubarak Seyed <se...@apple.com>.
> On June 30, 2012, 11:06 p.m., Mubarak Seyed wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java, line 188
> > <https://reviews.apache.org/r/5683/diff/1/?file=117627#file117627line188>
> >
> > single space needed before "ms"
>
> Mike Percy wrote:
> This is a pretty nitpicky comment, but I added the space anyway for ease of using awk/perl to parse the logs ;)
I added because some other parts of code prints with space. As you said, log mining and searching can be useful. Thanks for taking care of this.
- Mubarak
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5683/#review8773
-----------------------------------------------------------
On July 3, 2012, 7:35 p.m., Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5683/
> -----------------------------------------------------------
>
> (Updated July 3, 2012, 7:35 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Patch to add support for configurable connect and request timeout to Avro Sink. Also refactors some of the RpcClient libs to reduce the # of codepaths.
>
>
> This addresses bug FLUME-1316.
> https://issues.apache.org/jira/browse/FLUME-1316
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5c6d0e3
> flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 3765924
> flume-ng-doc/sphinx/FlumeUserGuide.rst 94f951f
> flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java 606a4bd
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java e304689
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java e19b093
> flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java e4f23a6
> flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 77bf331
>
> Diff: https://reviews.apache.org/r/5683/diff/
>
>
> Testing
> -------
>
> Added a unit test for the new params. Existing unit tests pass.
>
>
> Thanks,
>
> Mike Percy
>
>
Re: Review Request: FLUME-1316: AvroSink should be configurable for
connect-timeout and request-timeout
Posted by Mike Percy <mp...@gmail.com>.
> On June 30, 2012, 11:06 p.m., Mubarak Seyed wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java, line 188
> > <https://reviews.apache.org/r/5683/diff/1/?file=117627#file117627line188>
> >
> > single space needed before "ms"
>
> Mike Percy wrote:
> This is a pretty nitpicky comment, but I added the space anyway for ease of using awk/perl to parse the logs ;)
>
> Mubarak Seyed wrote:
> I added because some other parts of code prints with space. As you said, log mining and searching can be useful. Thanks for taking care of this.
No prob, thanks for the review Mubarak!
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/5683/#review8773
-----------------------------------------------------------
On July 3, 2012, 7:35 p.m., Mike Percy wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/5683/
> -----------------------------------------------------------
>
> (Updated July 3, 2012, 7:35 p.m.)
>
>
> Review request for Flume.
>
>
> Description
> -------
>
> Patch to add support for configurable connect and request timeout to Avro Sink. Also refactors some of the RpcClient libs to reduce the # of codepaths.
>
>
> This addresses bug FLUME-1316.
> https://issues.apache.org/jira/browse/FLUME-1316
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/AvroSink.java 5c6d0e3
> flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java 3765924
> flume-ng-doc/sphinx/FlumeUserGuide.rst 94f951f
> flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java 606a4bd
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java e304689
> flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientFactory.java e19b093
> flume-ng-sdk/src/test/java/org/apache/flume/api/RpcTestUtils.java e4f23a6
> flume-ng-sdk/src/test/java/org/apache/flume/api/TestNettyAvroRpcClient.java 77bf331
>
> Diff: https://reviews.apache.org/r/5683/diff/
>
>
> Testing
> -------
>
> Added a unit test for the new params. Existing unit tests pass.
>
>
> Thanks,
>
> Mike Percy
>
>