You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Miklos Csanady <mi...@cloudera.com> on 2017/07/17 14:20:04 UTC
Re: Review Request 57617: FLUME-2620 File Channel to support empty
values in headers
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57617/#review180678
-----------------------------------------------------------
Ship it!
Thank you for this path.
I tested the patch with your solution which seems to be good.
- Miklos Csanady
On March 16, 2017, 8 a.m., Marcell Hegedus wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57617/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 8 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2620
> https://issues.apache.org/jira/browse/FLUME-2620
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Flume user guide does not specify whether a value in event header could be null or not. Given an external system generating events which header vaules can be null and a user configures Flume with Memory Channel then he will have no trouble. Later on when the user changes Memory Channel to File Channel then Flume will fail with NPE. It is because FC is serializing events with protocol buffer and header values are defined as required in the proto file.
> In this patch I have changed the value field to optional. However protocol buffer does not have a notation for null and setting a field to null raises NPE again. Added a null check before serialization to prevent this.
> There is on caveat: When an optional field is not set, at deserialization it will be set to a default value: in this case it will be empty string.
>
>
> Diffs
> -----
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Put.java 0a70a24
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/proto/ProtosFactory.java 50492cc
> flume-ng-channels/flume-file-channel/src/main/proto/filechannel.proto 25520e8
> flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannel.java 8efe991
> flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java 344bb58
>
>
> Diff: https://reviews.apache.org/r/57617/diff/1/
>
>
> Testing
> -------
>
> Added unit test for both Memory Channel and File Channel to check if they accept null values in header.
>
>
> Thanks,
>
> Marcell Hegedus
>
>
Re: Review Request 57617: FLUME-2620 File Channel to support empty
values in headers
Posted by Denes Arvay <de...@cloudera.com>.
> On July 17, 2017, 4:20 p.m., Miklos Csanady wrote:
> > Thank you for this path.
> >
> > I tested the patch with your solution which seems to be good.
Thank you for the patch Marcell and for the review Miklos, I'm going to commit this if nobody has any concerns.
- Denes
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57617/#review180678
-----------------------------------------------------------
On March 16, 2017, 9 a.m., Marcell Hegedus wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57617/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 9 a.m.)
>
>
> Review request for Flume.
>
>
> Bugs: FLUME-2620
> https://issues.apache.org/jira/browse/FLUME-2620
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Flume user guide does not specify whether a value in event header could be null or not. Given an external system generating events which header vaules can be null and a user configures Flume with Memory Channel then he will have no trouble. Later on when the user changes Memory Channel to File Channel then Flume will fail with NPE. It is because FC is serializing events with protocol buffer and header values are defined as required in the proto file.
> In this patch I have changed the value field to optional. However protocol buffer does not have a notation for null and setting a field to null raises NPE again. Added a null check before serialization to prevent this.
> There is on caveat: When an optional field is not set, at deserialization it will be set to a default value: in this case it will be empty string.
>
>
> Diffs
> -----
>
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/Put.java 0a70a24
> flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/proto/ProtosFactory.java 50492cc
> flume-ng-channels/flume-file-channel/src/main/proto/filechannel.proto 25520e8
> flume-ng-channels/flume-file-channel/src/test/java/org/apache/flume/channel/file/TestFileChannel.java 8efe991
> flume-ng-core/src/test/java/org/apache/flume/channel/TestMemoryChannel.java 344bb58
>
>
> Diff: https://reviews.apache.org/r/57617/diff/1/
>
>
> Testing
> -------
>
> Added unit test for both Memory Channel and File Channel to check if they accept null values in header.
>
>
> Thanks,
>
> Marcell Hegedus
>
>