Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

Thanks, Yi!

On Wed, Jul 29, 2015 at 12:16 PM, Yi Pan  wrote:

> Hi, Roger,
>
> I am testing the patch now. Will update the JIRA soon.
>
> Thanks!
>
> -Yi
>
> On Wed, Jul 29, 2015 at 12:11 PM, Roger Hoover 
> wrote:
>
> > Thank you, Dan.  I think we're ready to merge.  Can one of the Samza
> > committers please take a look?
> >
> > On Wed, Jul 29, 2015 at 11:31 AM, Dan Harvey 
> > wrote:
> >
> > >This is an automatically generated e-mail. To reply, visit:
> > > https://reviews.apache.org/r/36815/
> > >
> > > On July 29th, 2015, 8:42 a.m. UTC, *Dan Harvey* wrote:
> > >
> > >
> > >
> >
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
> > > <
> >
> https://reviews.apache.org/r/36815/diff/6/?file=1024157#file1024157line116
> >
> > (Diff
> > > revision 6)
> > >
> > > public void register(final String source) {
> > >
> > >116
> > >
> > >   LOGGER.info(itemResp.getFailureMessage());
> > >
> > >   Should we add a Samza specifc message, then add the whole exception?
> > so it's more clear what the exception was from if the user doesn't know
> the
> > code? Logger.info("Failed to index message in ElasticSearch.", e);
> > >
> > > This would also be true for other log lines added.
> > >
> > >  On July 29th, 2015, 6:22 p.m. UTC, *Roger Hoover* wrote:
> > >
> > > Good idea.  Thanks.
> > >
> > > BTW, it didn't work like this: Logger.info("Failed to index message in
> > ElasticSearch.", itemResp.getFailure()) so I did this:
> > >
> > > LOGGER.error("Failed to index document in Elasticsearch: " +
> > itemResp.getFailureMessage());
> > >
> > >  On July 29th, 2015, 6:28 p.m. UTC, *Roger Hoover* wrote:
> > >
> > > This is what the messages look like
> > >
> > > 2015-07-29 11:15:02 ElasticsearchSystemProducer [INFO] Failed to index
> > document in Elasticsearch:
> > VersionConflictEngineException[[test-embedded.2015-07-29][0] [stuff][d3]:
> > version conflict, current [9], provided [5]]
> > >
> > >  That looks fine!
> > >
> > >
> > > - Dan
> > >
> > > On July 29th, 2015, 6:24 p.m. UTC, Roger Hoover wrote:
> > >   Review request for samza and Dan Harvey.
> > > By Roger Hoover.
> > >
> > > *Updated July 29, 2015, 6:24 p.m.*
> > >  *Repository: * samza
> > > Description
> > >
> > > SAMZA-741 Add support for versioning to Elasticsearch System Producer
> > >
> > >   Testing
> > >
> > > Refactored DefaultIndexRequestFactory to make it easier to subclass and
> > customize to handle version and version_type parameters.
> > >
> > >   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)
> > >
> > > View Diff 
> > >
> >
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Yi Pan]

Hi, Roger,

I am testing the patch now. Will update the JIRA soon.

Thanks!

-Yi

On Wed, Jul 29, 2015 at 12:11 PM, Roger Hoover 
wrote:

> Thank you, Dan.  I think we're ready to merge.  Can one of the Samza
> committers please take a look?
>
> On Wed, Jul 29, 2015 at 11:31 AM, Dan Harvey 
> wrote:
>
> >This is an automatically generated e-mail. To reply, visit:
> > https://reviews.apache.org/r/36815/
> >
> > On July 29th, 2015, 8:42 a.m. UTC, *Dan Harvey* wrote:
> >
> >
> >
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
> > <
> https://reviews.apache.org/r/36815/diff/6/?file=1024157#file1024157line116>
> (Diff
> > revision 6)
> >
> > public void register(final String source) {
> >
> >116
> >
> >   LOGGER.info(itemResp.getFailureMessage());
> >
> >   Should we add a Samza specifc message, then add the whole exception?
> so it's more clear what the exception was from if the user doesn't know the
> code? Logger.info("Failed to index message in ElasticSearch.", e);
> >
> > This would also be true for other log lines added.
> >
> >  On July 29th, 2015, 6:22 p.m. UTC, *Roger Hoover* wrote:
> >
> > Good idea.  Thanks.
> >
> > BTW, it didn't work like this: Logger.info("Failed to index message in
> ElasticSearch.", itemResp.getFailure()) so I did this:
> >
> > LOGGER.error("Failed to index document in Elasticsearch: " +
> itemResp.getFailureMessage());
> >
> >  On July 29th, 2015, 6:28 p.m. UTC, *Roger Hoover* wrote:
> >
> > This is what the messages look like
> >
> > 2015-07-29 11:15:02 ElasticsearchSystemProducer [INFO] Failed to index
> document in Elasticsearch:
> VersionConflictEngineException[[test-embedded.2015-07-29][0] [stuff][d3]:
> version conflict, current [9], provided [5]]
> >
> >  That looks fine!
> >
> >
> > - Dan
> >
> > On July 29th, 2015, 6:24 p.m. UTC, Roger Hoover wrote:
> >   Review request for samza and Dan Harvey.
> > By Roger Hoover.
> >
> > *Updated July 29, 2015, 6:24 p.m.*
> >  *Repository: * samza
> > Description
> >
> > SAMZA-741 Add support for versioning to Elasticsearch System Producer
> >
> >   Testing
> >
> > Refactored DefaultIndexRequestFactory to make it easier to subclass and
> customize to handle version and version_type parameters.
> >
> >   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)
> >
> > View Diff 
> >
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

Thank you, Dan.  I think we're ready to merge.  Can one of the Samza
committers please take a look?

On Wed, Jul 29, 2015 at 11:31 AM, Dan Harvey  wrote:

>This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
>
> On July 29th, 2015, 8:42 a.m. UTC, *Dan Harvey* wrote:
>
>
> samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
>  
> (Diff
> revision 6)
>
> public void register(final String source) {
>
>116
>
>   LOGGER.info(itemResp.getFailureMessage());
>
>   Should we add a Samza specifc message, then add the whole exception? so 
> it's more clear what the exception was from if the user doesn't know the 
> code? Logger.info("Failed to index message in ElasticSearch.", e);
>
> This would also be true for other log lines added.
>
>  On July 29th, 2015, 6:22 p.m. UTC, *Roger Hoover* wrote:
>
> Good idea.  Thanks.
>
> BTW, it didn't work like this: Logger.info("Failed to index message in 
> ElasticSearch.", itemResp.getFailure()) so I did this:
>
> LOGGER.error("Failed to index document in Elasticsearch: " + 
> itemResp.getFailureMessage());
>
>  On July 29th, 2015, 6:28 p.m. UTC, *Roger Hoover* wrote:
>
> This is what the messages look like
>
> 2015-07-29 11:15:02 ElasticsearchSystemProducer [INFO] Failed to index 
> document in Elasticsearch: 
> VersionConflictEngineException[[test-embedded.2015-07-29][0] [stuff][d3]: 
> version conflict, current [9], provided [5]]
>
>  That looks fine!
>
>
> - Dan
>
> On July 29th, 2015, 6:24 p.m. UTC, Roger Hoover wrote:
>   Review request for samza and Dan Harvey.
> By Roger Hoover.
>
> *Updated July 29, 2015, 6:24 p.m.*
>  *Repository: * samza
> Description
>
> SAMZA-741 Add support for versioning to Elasticsearch System Producer
>
>   Testing
>
> Refactored DefaultIndexRequestFactory to make it easier to subclass and 
> customize to handle version and version_type parameters.
>
>   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)
>
> View Diff 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Dan Harvey]

> On July 29, 2015, 8:42 a.m., Dan Harvey wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java,
> >  line 116
> > 
> >
> > Should we add a Samza specifc message, then add the whole exception? so 
> > it's more clear what the exception was from if the user doesn't know the 
> > code? `Logger.info("Failed to index message in ElasticSearch.", e);`
> > 
> > This would also be true for other log lines added.
> 
> Roger Hoover wrote:
> Good idea.  Thanks.
> 
> BTW, it didn't work like this: Logger.info("Failed to index message in 
> ElasticSearch.", itemResp.getFailure()) so I did this:
> 
> LOGGER.error("Failed to index document in Elasticsearch: " + 
> itemResp.getFailureMessage());
> 
> Roger Hoover wrote:
> This is what the messages look like
> 
> ```2015-07-29 11:15:02 ElasticsearchSystemProducer [INFO] Failed to index 
> document in Elasticsearch: 
> VersionConflictEngineException[[test-embedded.2015-07-29][0] [stuff][d3]: 
> version conflict, current [9], provided [5]]```

That looks fine!


- Dan


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93413
---


On July 29, 2015, 6:24 p.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 6:24 p.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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Yi Pan (Data Infrastructure)]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93493
---

Ship it!


LGTM. Thanks a lot!

- Yi Pan (Data Infrastructure)


On July 29, 2015, 6:24 p.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 6:24 p.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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

> On July 29, 2015, 8:42 a.m., Dan Harvey wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java,
> >  line 116
> > 
> >
> > Should we add a Samza specifc message, then add the whole exception? so 
> > it's more clear what the exception was from if the user doesn't know the 
> > code? `Logger.info("Failed to index message in ElasticSearch.", e);`
> > 
> > This would also be true for other log lines added.
> 
> Roger Hoover wrote:
> Good idea.  Thanks.
> 
> BTW, it didn't work like this: Logger.info("Failed to index message in 
> ElasticSearch.", itemResp.getFailure()) so I did this:
> 
> LOGGER.error("Failed to index document in Elasticsearch: " + 
> itemResp.getFailureMessage());

This is what the messages look like

```2015-07-29 11:15:02 ElasticsearchSystemProducer [INFO] Failed to index 
document in Elasticsearch: 
VersionConflictEngineException[[test-embedded.2015-07-29][0] [stuff][d3]: 
version conflict, current [9], provided [5]]```


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93413
---


On July 29, 2015, 6:24 p.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 6:24 p.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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/
---

(Updated July 29, 2015, 6:24 p.m.)


Review request for samza and Dan Harvey.


Changes
---

More descriptive log messages


Repository: samza


Description
---

SAMZA-741 Add support for versioning to Elasticsearch System Producer


Diffs (updated)
-

  
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

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

> On July 29, 2015, 8:42 a.m., Dan Harvey wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java,
> >  line 116
> > 
> >
> > Should we add a Samza specifc message, then add the whole exception? so 
> > it's more clear what the exception was from if the user doesn't know the 
> > code? `Logger.info("Failed to index message in ElasticSearch.", e);`
> > 
> > This would also be true for other log lines added.

Good idea.  Thanks.

BTW, it didn't work like this: Logger.info("Failed to index message in 
ElasticSearch.", itemResp.getFailure()) so I did this:

LOGGER.error("Failed to index document in Elasticsearch: " + 
itemResp.getFailureMessage());


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93413
---


On July 29, 2015, 6:22 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 6:22 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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Dan Harvey]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93413
---



samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 (line 116)


Should we add a Samza specifc message, then add the whole exception? so 
it's more clear what the exception was from if the user doesn't know the code? 
`Logger.info("Failed to index message in ElasticSearch.", e);`

This would also be true for other log lines added.


One comment on the logging but I think this looks great otherwise.

- Dan Harvey


On July 29, 2015, 6:22 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 6:22 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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/
---

(Updated July 29, 2015, 6:22 a.m.)


Review request for samza and Dan Harvey.


Changes
---

Making the invalid response type an error level log message


Repository: samza


Description
---

SAMZA-741 Add support for versioning to Elasticsearch System Producer


Diffs (updated)
-

  
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

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

> On July 29, 2015, 5:47 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java,
> >  line 149
> > 
> >
> > Quick question: Is it guaranteed that there is no DeleteResponse here? 
> > It would be good to at least log a warn if we get an unexpected response 
> > here.
> 
> Roger Hoover wrote:
> It is guaranteed that you will not get a DeleteResponse back because the 
> producer currently only allows IndexRequests.  In the furture, if it supports 
> DeleteRequest then we should add a counter metric for deletes.
> 
> Yi Pan (Data Infrastructure) wrote:
> @Roger, thanks for the explanation. My point is: it would be good to make 
> the code detect and log unexpected response.

Oh, I see.  I just added a warning log to do that.


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93395
---


On July 29, 2015, 6:22 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 6:22 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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/
---

(Updated July 29, 2015, 6:18 a.m.)


Review request for samza and Dan Harvey.


Changes
---

Added log warning for unexpected response types


Repository: samza


Description
---

SAMZA-741 Add support for versioning to Elasticsearch System Producer


Diffs (updated)
-

  
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

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Yi Pan (Data Infrastructure)]

> On July 29, 2015, 5:47 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java,
> >  line 149
> > 
> >
> > Quick question: Is it guaranteed that there is no DeleteResponse here? 
> > It would be good to at least log a warn if we get an unexpected response 
> > here.
> 
> Roger Hoover wrote:
> It is guaranteed that you will not get a DeleteResponse back because the 
> producer currently only allows IndexRequests.  In the furture, if it supports 
> DeleteRequest then we should add a counter metric for deletes.

@Roger, thanks for the explanation. My point is: it would be good to make the 
code detect and log unexpected response.


- Yi


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93395
---


On July 29, 2015, 5:17 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 5:17 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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

> On July 29, 2015, 5:47 a.m., Yi Pan (Data Infrastructure) wrote:
> > samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java,
> >  line 149
> > 
> >
> > Quick question: Is it guaranteed that there is no DeleteResponse here? 
> > It would be good to at least log a warn if we get an unexpected response 
> > here.

It is guaranteed that you will not get a DeleteResponse back because the 
producer currently only allows IndexRequests.  In the furture, if it supports 
DeleteRequest then we should add a counter metric for deletes.


- Roger


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93395
---


On July 29, 2015, 5:17 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 5:17 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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Yi Pan (Data Infrastructure)]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/#review93395
---


LGTM except a nit comment. Thanks!


samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 (line 147)


Quick question: Is it guaranteed that there is no DeleteResponse here? It 
would be good to at least log a warn if we get an unexpected response here.


- Yi Pan (Data Infrastructure)


On July 29, 2015, 5:17 a.m., Roger Hoover wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36815/
> ---
> 
> (Updated July 29, 2015, 5:17 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
> 
>

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/
---

(Updated July 29, 2015, 5:17 a.m.)


Review request for samza and Dan Harvey.


Changes
---

Updates based on Dan's feedback


Repository: samza


Description
---

SAMZA-741 Add support for versioning to Elasticsearch System Producer


Diffs (updated)
-

  
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

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

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

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Dan Harvey]

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


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)


I don't think the LOGGER check here is needed?



samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/ElasticsearchSystemProducer.java
 (line 118)


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)


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)


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)


.equals()



samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/indexrequest/DefaultIndexRequestFactory.java
 (line 92)


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

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/
---

(Updated July 28, 2015, 6:13 a.m.)


Review request for samza.


Changes
---

Ignoring version conflicts


Repository: samza


Description
---

SAMZA-741 Add support for versioning to Elasticsearch System Producer


Diffs (updated)
-

  
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

Re: Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/
---

(Updated July 25, 2015, 4:48 p.m.)


Review request for samza.


Repository: samza


Description
---

SAMZA-741 Add support for versioning to Elasticsearch System Producer


Diffs (updated)
-

  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/indexrequest/DefaultIndexRequestFactory.java
 afe0eee 

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

Review Request 36815: SAMZA-741 Support for versioning with Elasticsearch Producer

[Roger Hoover]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36815/
---

Review request for samza.


Repository: samza


Description
---

SAMZA-741 Add support for versioning to Elasticsearch System Producer


Diffs
-

  
samza-elasticsearch/src/main/java/org/apache/samza/system/elasticsearch/indexrequest/DefaultIndexRequestFactory.java
 afe0eee 

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