----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10380/#review18944 -----------------------------------------------------------
Mr. Sargisson, Thanks for posting this review. >From an elasticsearch HTTP client perspective, the patch looks good. However, in your patch, the only class that implements ElasticSearchIdProvider is ElasticSearchNullIdProvider and it returns a null id which means ES is still going to have to generate one when the document is indexed. Based on the description of the patch reviewed, it would have been nice to see another example class that implements ElasticSearchIdProvider that actually generates a non-null document id so that this default behavior can be overriden. So maybe generating a document id based on the original timestamp in the event optionally in combination of other headers would be nice. Nevertheless, this is a good effort. Thanks. - Israel Ekpo On April 9, 2013, 5:59 p.m., Edward Sargisson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10380/ > ----------------------------------------------------------- > > (Updated April 9, 2013, 5:59 p.m.) > > > Review request for Flume and Hari Shreedharan. > > > Description > ------- > > Elasticsearch allows clients to specify the id of the newly added document - > and will generate one by default if none is supplied. If the user knows that > the incoming event has a unique id then that id can be used as the > elasticsearch document id. This change enhances the ElasticSearchSink to > allow users to provide a custom class to decide what the id should be. > > > This addresses bug FLUME-1972. > https://issues.apache.org/jira/browse/FLUME-1972 > > > Diffs > ----- > > flume-ng-doc/sphinx/FlumeUserGuide.rst 693c0d7 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchIdProvider.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchNullIdProvider.java > PRE-CREATION > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java > 1b3db14 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSinkConstants.java > 7f75e22 > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/AbstractElasticSearchSinkTest.java > 2edacdc > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java > 94b95b1 > > Diff: https://reviews.apache.org/r/10380/diff/ > > > Testing > ------- > > All unit tests and integration tests pass. A snapshot using commit > 5b9d31f1ad228 and the patch for flume-1972 has passed our internal > integration tests using customisations. > > > Thanks, > > Edward Sargisson > >
