You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Benjamin Fiorini <be...@gmail.com> on 2015/04/15 11:56:32 UTC
Review Request 33215: FLUME-2649 ElasticSearch sink should index
nested JSON
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33215/
-----------------------------------------------------------
Review request for Flume.
Repository: flume-git
Description
-------
Elasticsearch sink fails to index malformed nested JSON
Diffs
-----
flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java de0acf4
flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java 2f8fd6d
flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java 78e1665
Diff: https://reviews.apache.org/r/33215/diff/
Testing
-------
Thanks,
Benjamin Fiorini
Re: Review Request 33215: FLUME-2649 ElasticSearch sink should index
nested JSON
Posted by Benjamin Fiorini <be...@gmail.com>.
> On April 15, 2015, 6:41 p.m., Francis Page wrote:
> > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java, line 64
> > <https://reviews.apache.org/r/33215/diff/1/?file=929718#file929718line64>
> >
> > You don't have to create a tmp builder. To make sure the JSON is valid, you just have to do:
> > while(parser.nextToken() != null) { }
True, cause that's exactly what copyCurrentStructure does under the hood (recursively).
> On April 15, 2015, 6:41 p.m., Francis Page wrote:
> > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java, line 44
> > <https://reviews.apache.org/r/33215/diff/1/?file=929718#file929718line44>
> >
> > This should not only check if it's null, but also if it's a json content type.
Not sure about this one, if it's JSON/YAML/SMILE, then contentType is not going to be null and the data can be parsed as a complex field. IMHO this does the job well.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33215/#review80205
-----------------------------------------------------------
On April 15, 2015, 11:56 a.m., Benjamin Fiorini wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33215/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 11:56 a.m.)
>
>
> Review request for Flume.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Elasticsearch sink fails to index malformed nested JSON
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java de0acf4
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java 2f8fd6d
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java 78e1665
>
> Diff: https://reviews.apache.org/r/33215/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Fiorini
>
>
Re: Review Request 33215: FLUME-2649 ElasticSearch sink should index
nested JSON
Posted by Francis Page <fr...@ubisoft.com>.
> On April 15, 2015, 4:41 p.m., Francis Page wrote:
> > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java, line 44
> > <https://reviews.apache.org/r/33215/diff/1/?file=929718#file929718line44>
> >
> > This should not only check if it's null, but also if it's a json content type.
>
> Benjamin Fiorini wrote:
> Not sure about this one, if it's JSON/YAML/SMILE, then contentType is not going to be null and the data can be parsed as a complex field. IMHO this does the job well.
You're right, forget about this!
- Francis
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33215/#review80205
-----------------------------------------------------------
On April 15, 2015, 9:56 a.m., Benjamin Fiorini wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33215/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 9:56 a.m.)
>
>
> Review request for Flume.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Elasticsearch sink fails to index malformed nested JSON
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java de0acf4
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java 2f8fd6d
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java 78e1665
>
> Diff: https://reviews.apache.org/r/33215/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Fiorini
>
>
Re: Review Request 33215: FLUME-2649 ElasticSearch sink should index
nested JSON
Posted by Francis Page <fr...@ubisoft.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33215/#review80205
-----------------------------------------------------------
flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java
<https://reviews.apache.org/r/33215/#comment129990>
This should not only check if it's null, but also if it's a json content type.
flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java
<https://reviews.apache.org/r/33215/#comment129991>
You don't have to create a tmp builder. To make sure the JSON is valid, you just have to do:
while(parser.nextToken() != null) { }
- Francis Page
On April 15, 2015, 9:56 a.m., Benjamin Fiorini wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33215/
> -----------------------------------------------------------
>
> (Updated April 15, 2015, 9:56 a.m.)
>
>
> Review request for Flume.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Elasticsearch sink fails to index malformed nested JSON
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java de0acf4
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java 2f8fd6d
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java 78e1665
>
> Diff: https://reviews.apache.org/r/33215/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Fiorini
>
>
Re: Review Request 33215: FLUME-2649 ElasticSearch sink should index
nested JSON
Posted by Benjamin Fiorini <be...@gmail.com>.
> On April 16, 2015, 5:54 p.m., Edward Sargisson wrote:
> > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java, line 58
> > <https://reviews.apache.org/r/33215/diff/2/?file=931607#file931607line58>
> >
> > To my mind, at this point I would suggest a broader comment to explain the gyrations we've been going through. How about something like:
> > Elasticsearch will accept JSON directly but we need to validate that the incoming event is JSON first. Sadly, the elasticsearch JSON parser is a stream parser so we need to instantiate it, parse the event to validate it, then instantiate it again to provide the JSON to elasticsearch.
> > If validation fails then the incoming event is submitted to elasticsearch as plain text.
Sounds good
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33215/#review80336
-----------------------------------------------------------
On April 16, 2015, 2:31 p.m., Benjamin Fiorini wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33215/
> -----------------------------------------------------------
>
> (Updated April 16, 2015, 2:31 p.m.)
>
>
> Review request for Flume.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Elasticsearch sink fails to index malformed nested JSON
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java de0acf4
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java 2f8fd6d
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java 78e1665
>
> Diff: https://reviews.apache.org/r/33215/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Fiorini
>
>
Re: Review Request 33215: FLUME-2649 ElasticSearch sink should index
nested JSON
Posted by Edward Sargisson <es...@pobox.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33215/#review80336
-----------------------------------------------------------
flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java
<https://reviews.apache.org/r/33215/#comment130183>
To my mind, at this point I would suggest a broader comment to explain the gyrations we've been going through. How about something like:
Elasticsearch will accept JSON directly but we need to validate that the incoming event is JSON first. Sadly, the elasticsearch JSON parser is a stream parser so we need to instantiate it, parse the event to validate it, then instantiate it again to provide the JSON to elasticsearch.
If validation fails then the incoming event is submitted to elasticsearch as plain text.
- Edward Sargisson
On April 16, 2015, 5:31 a.m., Benjamin Fiorini wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33215/
> -----------------------------------------------------------
>
> (Updated April 16, 2015, 5:31 a.m.)
>
>
> Review request for Flume.
>
>
> Repository: flume-git
>
>
> Description
> -------
>
> Elasticsearch sink fails to index malformed nested JSON
>
>
> Diffs
> -----
>
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java de0acf4
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java 2f8fd6d
> flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java 78e1665
>
> Diff: https://reviews.apache.org/r/33215/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Benjamin Fiorini
>
>
Re: Review Request 33215: FLUME-2649 ElasticSearch sink should index
nested JSON
Posted by Benjamin Fiorini <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33215/
-----------------------------------------------------------
(Updated April 17, 2015, 9:22 a.m.)
Review request for Flume.
Changes
-------
Better comment.
Repository: flume-git
Description
-------
Elasticsearch sink fails to index malformed nested JSON
Diffs (updated)
-----
flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java de0acf4
flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java 2f8fd6d
flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java 78e1665
Diff: https://reviews.apache.org/r/33215/diff/
Testing
-------
Thanks,
Benjamin Fiorini
Re: Review Request 33215: FLUME-2649 ElasticSearch sink should index
nested JSON
Posted by Benjamin Fiorini <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/33215/
-----------------------------------------------------------
(Updated April 16, 2015, 2:31 p.m.)
Review request for Flume.
Changes
-------
Fixed after Francis Page's comment
Repository: flume-git
Description
-------
Elasticsearch sink fails to index malformed nested JSON
Diffs (updated)
-----
flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ContentBuilderUtil.java de0acf4
flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java 2f8fd6d
flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java 78e1665
Diff: https://reviews.apache.org/r/33215/diff/
Testing
-------
Thanks,
Benjamin Fiorini