----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36815/#review93249 -----------------------------------------------------------
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java (line 115) <https://reviews.apache.org/r/36815/#comment147599> could switch these around so you've got getStatus().equals(RestStatus.CONFLICT), fewer nots. samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java (line 117) <https://reviews.apache.org/r/36815/#comment147601> I don't think the LOGGER check here is needed? samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java (line 118) <https://reviews.apache.org/r/36815/#comment147600> 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? samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java (line 124) <https://reviews.apache.org/r/36815/#comment147602> 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? samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java (line 135) <https://reviews.apache.org/r/36815/#comment147595> This already gets logged and thrown in flush(), is this to see the exception sooner? samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java (line 174) <https://reviews.apache.org/r/36815/#comment147603> .equals() samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/indexrequest/DefaultIndexRequestFactory.java (line 92) <https://reviews.apache.org/r/36815/#comment147604> I know I set this line before but reading the elasticsearch docs they recommend using `Requests.indexRequest(index).type(type)` - Dan Harvey 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 > >
