> On July 28, 2015, 7:36 a.m., Dan Harvey wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java, > > line 115 > > <https://reviews.apache.org/r/36815/diff/3/?file=1023400#file1023400line115> > > > > could switch these around so you've got > > getStatus().equals(RestStatus.CONFLICT), fewer nots.
Ok. I reworked it to not have nots. :) > On July 28, 2015, 7:36 a.m., Dan Harvey wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java, > > line 117 > > <https://reviews.apache.org/r/36815/diff/3/?file=1023400#file1023400line117> > > > > I don't think the LOGGER check here is needed? Yeah, not necessary here. > On July 28, 2015, 7:36 a.m., Dan Harvey wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java, > > line 118 > > <https://reviews.apache.org/r/36815/diff/3/?file=1023400#file1023400line118> > > > > I think it's worth adding a metrics here too so we know how many > > conflicts occur. Then does the log message from elastic search fit in with > > the Sazma ones, and is easy to understand? Added them in the updateSuccessMetrics() method > On July 28, 2015, 7:36 a.m., Dan Harvey wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java, > > line 124 > > <https://reviews.apache.org/r/36815/diff/3/?file=1023400#file1023400line124> > > > > it is odd a method with the name updateSuccessMetrics returns the > > number of writes? could just leave the log line as it was? > > > > or it could compute the different in the metrics before and after > > calling updateSuccessMetrics() here? might make more sense? Leaving it as is seemed misleading since version conflicts are not successfully written. Before/after comparison seems a little messy so I moved the log line to the updateSuccessMetrics() method > On July 28, 2015, 7:36 a.m., Dan Harvey wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/indexrequest/DefaultIndexRequestFactory.java, > > line 92 > > <https://reviews.apache.org/r/36815/diff/3/?file=1023402#file1023402line92> > > > > I know I set this line before but reading the elasticsearch docs they > > recommend using `Requests.indexRequest(index).type(type)` Sounds good. > On July 28, 2015, 7:36 a.m., Dan Harvey wrote: > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java, > > line 139 > > <https://reviews.apache.org/r/36815/diff/3/?file=1023400#file1023400line139> > > > > This already gets logged and thrown in flush(), is this to see the > > exception sooner? These are the individual exceptions per document. They're not being logged in the flush() method. Only batch-level errors are saved and logged in the flush. I wasn't seeing any of the document level errors in the log. - Roger ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36815/#review93249 ----------------------------------------------------------- On July 28, 2015, 6:15 a.m., Roger Hoover wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36815/ > ----------------------------------------------------------- > > (Updated July 28, 2015, 6:15 a.m.) > > > Review request for samza and Dan Harvey. > > > Repository: samza > > > Description > ------- > > SAMZA-741 Add support for versioning to Elasticsearch System Producer > > > Diffs > ----- > > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java > f61bd36 > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetrics.java > e3b635b > > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/indexrequest/DefaultIndexRequestFactory.java > afe0eee > > samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerMetricsTest.java > 980964f > > samza-elasticsearch/src/test/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducerTest.java > 684d7f6 > > Diff: https://reviews.apache.org/r/36815/diff/ > > > Testing > ------- > > Refactored DefaultIndexRequestFactory to make it easier to subclass and > customize to handle version and version_type parameters. > > > Thanks, > > Roger Hoover > >
