samza-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Harvey" <danharve...@gmail.com>
Subject Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer
Date Tue, 28 Jul 2015 07:36:33 GMT

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


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