> On April 11, 2013, 10:28 a.m., Israel Ekpo wrote: > > flume-ng-sinks/flume-ng-elasticsearch-sink/src/main/java/org/apache/flume/sink/elasticsearch/ElasticSearchSink.java, > > line 248 > > <https://reviews.apache.org/r/10379/diff/1/?file=279376#file279376line248> > > > > Since you are retrieving the headers twice within the method, you could > > retrieve it once and store it in a final variable for use later. > > > > Also perform null checks before attempting to use the variable. > > > > There may be cases where the event has no headers.
Hi Mr Ekpo, The current structure of the code, where ensureHasTimestamp will create the headers map if required, means that this idea probably won't work as nicely as it sounds. We'd end up having to do the "create map if missing code" in the process method itself - which seems odd. In my judgment, the small cost of an additional lookup on the object is worth it compared to the memory cost of hanging onto an additional temporary reference. I'd be incredibly surprised if any performance difference were found - and I could argue that the additional temporary reference version is slower because it would leave more garbage requiring longer collection time. - Edward ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10379/#review18998 ----------------------------------------------------------- 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/10379/ > ----------------------------------------------------------- > > (Updated April 9, 2013, 5:59 p.m.) > > > Review request for Flume and Hari Shreedharan. > > > Description > ------- > > This change gets the timestamp from the event and uses it (in UTC) to > determine the name of the index to write to. This is required to match the > behaviour of Logstash so that Kibana can find the log events. > The previous code would use the current time and would do it in the timezone > of the Flume agent's host. > > > This addresses bug FLUME-1782. > https://issues.apache.org/jira/browse/FLUME-1782 > > > Diffs > ----- > > flume-ng-doc/sphinx/FlumeUserGuide.rst 693c0d7 > > 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/test/java/org/apache/flume/sink/elasticsearch/TestElasticSearchSink.java > 94b95b1 > > Diff: https://reviews.apache.org/r/10379/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 > >
