samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Roger Hoover" <roger.hoo...@gmail.com>
Subject Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer
Date Wed, 29 Jul 2015 05:15:29 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message