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/29 16:35:09 UTC
Re: Review Request: FLUME-872 - Timestamp header be a standard part of an
event
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/3088/#review5458
-----------------------------------------------------------
Hi, in general it looks good!
Please remove the extra spaces, they show up in red.
Some large changes have been merged in. Rebasing on trunk might be worthwhile.
/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java
<https://reviews.apache.org/r/3088/#comment11857>
People often assume getters are very light weight and call them more often required. Should we store the value as an member variable as well as in the map as opposed to parsing the string each time?
/branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java
<https://reviews.apache.org/r/3088/#comment11861>
Original code uses "" + but you use Long.toString below. Should we change it to the more proper Long.toString?
/branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java
<https://reviews.apache.org/r/3088/#comment11860>
Most of the code does not use static imports. I am not used to this myself, but it's good to be consistent.
- Brock
On 2011-12-09 04:21:34, Vibul Imtarnasan wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/3088/
> -----------------------------------------------------------
>
> (Updated 2011-12-09 04:21:34)
>
>
> Review request for Flume.
>
>
> Summary
> -------
>
> FLUME-872
>
>
> Diffs
> -----
>
> /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSTextFormatter.java 1212231
> /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSWritableFormatter.java 1212231
> /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/event/EventBuilder.java 1212231
> /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/event/SimpleEvent.java 1212231
> /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/formatter/output/BucketPath.java 1212231
> /branches/flume-728/flume-ng-core/src/test/java/org/apache/flume/event/TestSimpleEvent.java PRE-CREATION
> /branches/flume-728/flume-ng-sinks/flume-hdfs-sink/src/main/java/org/apache/flume/sink/hdfs/HDFSEventSink.java 1212231
> /branches/flume-728/flume-ng-core/src/main/java/org/apache/flume/Event.java 1212231
>
> Diff: https://reviews.apache.org/r/3088/diff
>
>
> Testing
> -------
>
> Unit test cases included
>
>
> Thanks,
>
> Vibul
>
>