You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Ashish Paliwal <pa...@gmail.com> on 2013/12/11 10:58:29 UTC
Review Request 16179: Patch for FLUME-2246 - event body data size can make
it configurable for logger sinker
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16179/
-----------------------------------------------------------
Review request for Flume.
Bugs: FLUME-2246
https://issues.apache.org/jira/browse/FLUME-2246
Repository: flume-git
Description
-------
Added configuration options to let user select the max bytes to dump in log file. Default stays at 16 bytes.
Change summary
Made LoggerSink implement configurable
user can provide maxByteToDump using configuration
User Guide updated
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 128fa84
flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java 92ff6fe
flume-ng-doc/sphinx/FlumeUserGuide.rst ae66f89
Diff: https://reviews.apache.org/r/16179/diff/
Testing
-------
Added test case that sets option to a value higher than 30 and use a string of length greater than 16 in output.
Thanks,
Ashish Paliwal
Re: Review Request 16179: Patch for FLUME-2246 - event body data size
can make it configurable for logger sinker
Posted by Roshan Naik <ro...@hortonworks.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16179/#review62319
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java
<https://reviews.apache.org/r/16179/#comment104346>
Lets fix the asterisks import since that seems to be covention adopted by Flume.
flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java
<https://reviews.apache.org/r/16179/#comment104345>
Consider renaming to "maxBytesToLog"
flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java
<https://reviews.apache.org/r/16179/#comment104344>
maxBytesToDump is not being reverted to default in the event of exception.
This can happen during reconfigure when going from a previous non-default value to a new badly speciifed value.
- Roshan Naik
On Dec. 11, 2013, 9:58 a.m., Ashish Paliwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16179/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2013, 9:58 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2246
> https://issues.apache.org/jira/browse/FLUME-2246
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Added configuration options to let user select the max bytes to dump in log file. Default stays at 16 bytes.
>
> Change summary
> Made LoggerSink implement configurable
> user can provide maxByteToDump using configuration
> User Guide updated
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 128fa84
> flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java 92ff6fe
> flume-ng-doc/sphinx/FlumeUserGuide.rst ae66f89
>
> Diff: https://reviews.apache.org/r/16179/diff/
>
>
> Testing
> -------
>
> Added test case that sets option to a value higher than 30 and use a string of length greater than 16 in output.
>
>
> Thanks,
>
> Ashish Paliwal
>
>
Re: Review Request 16179: Patch for FLUME-2246 - event body data size
can make it configurable for logger sinker
Posted by Ashish Paliwal <pa...@gmail.com>.
> On Oct. 11, 2014, 8:11 p.m., Jarek Cecho wrote:
> > flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java, lines 68-89
> > <https://reviews.apache.org/r/16179/diff/1/?file=396539#file396539line68>
> >
> > I'm missing validation of inserted data. Shouldn't there be some assert statements?
> >
> > Also the test itself can be probably enhanced. What about having constant message (10 bytes) and changing the maxBytesToDump option from 1 to 20 to ensure that we get on the output expected outcome?
This is LoggerSink, writing to a file, not sure how to intercept written message and apply an assert on it. The test is built upon an existing test case. Any pointers?
- Ashish
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16179/#review56298
-----------------------------------------------------------
On Dec. 11, 2013, 9:58 a.m., Ashish Paliwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16179/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2013, 9:58 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2246
> https://issues.apache.org/jira/browse/FLUME-2246
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Added configuration options to let user select the max bytes to dump in log file. Default stays at 16 bytes.
>
> Change summary
> Made LoggerSink implement configurable
> user can provide maxByteToDump using configuration
> User Guide updated
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 128fa84
> flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java 92ff6fe
> flume-ng-doc/sphinx/FlumeUserGuide.rst ae66f89
>
> Diff: https://reviews.apache.org/r/16179/diff/
>
>
> Testing
> -------
>
> Added test case that sets option to a value higher than 30 and use a string of length greater than 16 in output.
>
>
> Thanks,
>
> Ashish Paliwal
>
>
Re: Review Request 16179: Patch for FLUME-2246 - event body data size
can make it configurable for logger sinker
Posted by Jarek Cecho <ja...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16179/#review56298
-----------------------------------------------------------
flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java
<https://reviews.apache.org/r/16179/#comment96609>
Please don't use asterisks import, enumerate what exactly do you need.
flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java
<https://reviews.apache.org/r/16179/#comment96611>
I'm missing validation of inserted data. Shouldn't there be some assert statements?
Also the test itself can be probably enhanced. What about having constant message (10 bytes) and changing the maxBytesToDump option from 1 to 20 to ensure that we get on the output expected outcome?
flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java
<https://reviews.apache.org/r/16179/#comment96610>
I'm assuming that this main class should not be part of the final patch?
- Jarek Cecho
On Dec. 11, 2013, 9:58 a.m., Ashish Paliwal wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16179/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2013, 9:58 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2246
> https://issues.apache.org/jira/browse/FLUME-2246
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Added configuration options to let user select the max bytes to dump in log file. Default stays at 16 bytes.
>
> Change summary
> Made LoggerSink implement configurable
> user can provide maxByteToDump using configuration
> User Guide updated
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/sink/LoggerSink.java 128fa84
> flume-ng-core/src/test/java/org/apache/flume/sink/TestLoggerSink.java 92ff6fe
> flume-ng-doc/sphinx/FlumeUserGuide.rst ae66f89
>
> Diff: https://reviews.apache.org/r/16179/diff/
>
>
> Testing
> -------
>
> Added test case that sets option to a value higher than 30 and use a string of length greater than 16 in output.
>
>
> Thanks,
>
> Ashish Paliwal
>
>