You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Brock Noland <br...@cloudera.com> on 2012/02/16 21:32:40 UTC
Review Request: FLUME-828: LoggerSink representation of the event's body
isn't too useful
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3928/
-----------------------------------------------------------
Review request for Flume.
Summary
-------
Changes SimpleEvent.toString() to include a human readable representation of the body byte array.
This addresses bug FLUME-828.
https://issues.apache.org/jira/browse/FLUME-828
Diffs
-----
flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java e0c3b45
flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java PRE-CREATION
Diff: https://reviews.apache.org/r/3928/diff
Testing
-------
Added two tests for the new code.
Thanks,
Brock
Re: Review Request: FLUME-828: LoggerSink representation of the event's body
isn't too useful
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3928/#review6201
-----------------------------------------------------------
Ship it!
+1
Thanks for the patch Brock. Can you please rebase it to the trunk and attache it to the Jira.
- Arvind
On 2012-02-28 13:46:28, Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3928/
> -----------------------------------------------------------
>
> (Updated 2012-02-28 13:46:28)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Changes SimpleEvent.toString() to include a human readable representation of the body byte array.
>
>
> This addresses bug FLUME-828.
> https://issues.apache.org/jira/browse/FLUME-828
>
>
> Diffs
> -----
>
> flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java e0c3b45
>
> Diff: https://reviews.apache.org/r/3928/diff
>
>
> Testing
> -------
>
> Added two tests for the new code.
>
>
> Thanks,
>
> Brock
>
>
Re: Review Request: FLUME-828: LoggerSink representation of the event's body
isn't too useful
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3928/
-----------------------------------------------------------
(Updated 2012-02-28 13:46:28.195492)
Review request for Flume.
Changes
-------
Thanks for the comments! I addressed them in the updated patch/comments.
Summary
-------
Changes SimpleEvent.toString() to include a human readable representation of the body byte array.
This addresses bug FLUME-828.
https://issues.apache.org/jira/browse/FLUME-828
Diffs (updated)
-----
flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java PRE-CREATION
flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java e0c3b45
Diff: https://reviews.apache.org/r/3928/diff
Testing
-------
Added two tests for the new code.
Thanks,
Brock
Re: Review Request: FLUME-828: LoggerSink representation of the event's body
isn't too useful
Posted by Brock Noland <br...@cloudera.com>.
> On 2012-02-28 01:06:14, Arvind Prabhakar wrote:
> > Thanks for the patch Brock. Some comments follow:
> >
> > * Please remove any trailing whitespaces from the code where present. These are highlighted in red on the review board.
Sorry about that, created this patch before I saw the policy.
> On 2012-02-28 01:06:14, Arvind Prabhakar wrote:
> > flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java, line 66
> > <https://reviews.apache.org/r/3928/diff/2/?file=75703#file75703line66>
> >
> > Need to guard against a null body.
Fixed
> On 2012-02-28 01:06:14, Arvind Prabhakar wrote:
> > flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java, line 78
> > <https://reviews.apache.org/r/3928/diff/2/?file=75703#file75703line78>
> >
> > would be good to log this exception as well.
Fixed
> On 2012-02-28 01:06:14, Arvind Prabhakar wrote:
> > flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java, line 71
> > <https://reviews.apache.org/r/3928/diff/2/?file=75703#file75703line71>
> >
> > This is the same variable name as what is used on line 80. Please use a different name for avoiding confusion.
> >
> > Also, ideally, the character encoding should be read from a system header value within the event. If the header is not present, the default encoding of UTF-8 could be used. That way the representation stays flexible.
This String is coming from Hexdump.dump. I looked and it does not specify a character set while building the string so it will get the default. I removed the charset so we will get the same default.
- Brock
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3928/#review5375
-----------------------------------------------------------
On 2012-02-28 13:46:28, Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3928/
> -----------------------------------------------------------
>
> (Updated 2012-02-28 13:46:28)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Changes SimpleEvent.toString() to include a human readable representation of the body byte array.
>
>
> This addresses bug FLUME-828.
> https://issues.apache.org/jira/browse/FLUME-828
>
>
> Diffs
> -----
>
> flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java PRE-CREATION
> flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java e0c3b45
>
> Diff: https://reviews.apache.org/r/3928/diff
>
>
> Testing
> -------
>
> Added two tests for the new code.
>
>
> Thanks,
>
> Brock
>
>
Re: Review Request: FLUME-828: LoggerSink representation of the event's body
isn't too useful
Posted by Arvind Prabhakar <ar...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3928/#review5375
-----------------------------------------------------------
Thanks for the patch Brock. Some comments follow:
* Please remove any trailing whitespaces from the code where present. These are highlighted in red on the review board.
flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java
<https://reviews.apache.org/r/3928/#comment11704>
Need to guard against a null body.
flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java
<https://reviews.apache.org/r/3928/#comment11706>
This is the same variable name as what is used on line 80. Please use a different name for avoiding confusion.
Also, ideally, the character encoding should be read from a system header value within the event. If the header is not present, the default encoding of UTF-8 could be used. That way the representation stays flexible.
flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java
<https://reviews.apache.org/r/3928/#comment11705>
would be good to log this exception as well.
- Arvind
On 2012-02-17 19:51:58, Brock Noland wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3928/
> -----------------------------------------------------------
>
> (Updated 2012-02-17 19:51:58)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> Changes SimpleEvent.toString() to include a human readable representation of the body byte array.
>
>
> This addresses bug FLUME-828.
> https://issues.apache.org/jira/browse/FLUME-828
>
>
> Diffs
> -----
>
> flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java e0c3b45
> flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/3928/diff
>
>
> Testing
> -------
>
> Added two tests for the new code.
>
>
> Thanks,
>
> Brock
>
>
Re: Review Request: FLUME-828: LoggerSink representation of the event's body
isn't too useful
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3928/
-----------------------------------------------------------
(Updated 2012-02-17 19:51:58.850062)
Review request for Flume.
Changes
-------
Felt guilt about using an anonymous class when ByteArrayOutputStream would suffice. Updated patch.
Summary
-------
Changes SimpleEvent.toString() to include a human readable representation of the body byte array.
This addresses bug FLUME-828.
https://issues.apache.org/jira/browse/FLUME-828
Diffs (updated)
-----
flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java e0c3b45
flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java PRE-CREATION
Diff: https://reviews.apache.org/r/3928/diff
Testing
-------
Added two tests for the new code.
Thanks,
Brock