Re: Review Request 65942: SOLR-11982: Add support for shards.sort parameter

[Tomás Fernández Löbbe]

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

(Updated March 28, 2018, 3:19 a.m.)


Review request for lucene.


Changes
---

Uploading most recent patch from SOLR-11982 on behalf of Ere Maijala


Repository: lucene-solr


Description
---

Creating this Review request on Ere Maijala's patch. See SOLR-11982 for 
previous discussion.
It would be nice to have the possibility to easily sort the shards in the 
preferred order e.g. by replica type. The attached patch adds support for 
shards.sort parameter that allows one to sort e.g. PULL and TLOG replicas first 
with ``shards.sor=replicaType:PULL|TLOG``(which would mean that NRT replicas 
wouldn't be hit with queries unless they're the only ones available) and/or to 
sort by replica location (like preferLocalShards=true but more versatile).


Diffs (updated)
-

  solr/CHANGES.txt 4af91ea76c 
  
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
 6bfd36af94 
  solr/core/src/java/org/apache/solr/util/TimeOut.java ce996f4326 
  
solr/core/src/test/org/apache/solr/handler/component/TestHttpShardHandlerFactory.java
 3ffa015a26 
  solr/solr-ref-guide/src/distributed-requests.adoc 096f632bbd 
  solr/solr-ref-guide/src/shards-and-indexing-data-in-solrcloud.adoc 81c6f8 
  solr/solrj/src/java/org/apache/solr/common/params/ShardParams.java cbc33f41f4 
  
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java 
e54f9ad7c6 


Diff: https://reviews.apache.org/r/65942/diff/3/

Changes: https://reviews.apache.org/r/65942/diff/2-3/


Testing
---


Thanks,

Tomás Fernández Löbbe

Re: Review Request 65942: SOLR-11982: Add support for shards.sort parameter

[Ere Maijala]

> On maalis 8, 2018, 8:54 ip, Tomás Fernández Löbbe wrote:
> >

I believe I've addressed the comments.


> On maalis 8, 2018, 8:54 ip, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Lines 309 (patched)
> > 
> >
> > I'm debating with myself on whether we should allow both params to be 
> > specified or not. I think it would be better to throw an exception in case 
> > of both parameters specified (since preferLocalShards would now be 
> > deprecated)

Agreed. The parameters are now mutually exclusive.


> On maalis 8, 2018, 8:54 ip, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Line 364 (original), 325 (patched)
> > 
> >
> > I think we should surround this with a try/catch and throw a 
> > SolrException(SolrError.BAD_REQUEST) in case of an IllegalArgumentException

Done.


> On maalis 8, 2018, 8:54 ip, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Lines 379 (patched)
> > 
> >
> > This should be an inner class or have it's own file

Right, fixed.


> On maalis 8, 2018, 8:54 ip, Tomás Fernández Löbbe wrote:
> > solr/core/src/test/org/apache/solr/handler/component/HttpShardHandlerFactoryTest.java
> > Lines 31 (patched)
> > 
> >
> > Note that there is a `TestHttpShardHandlerFactory`, did you 
> > intentionally not use that one?

No, I just failed to find it due to its naming. Fixed.


- Ere


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


On maalis 8, 2018, 5:38 ip, Tomás Fernández Löbbe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65942/
> ---
> 
> (Updated maalis 8, 2018, 5:38 ip)
> 
> 
> Review request for lucene.
> 
> 
> Repository: lucene-solr
> 
> 
> Description
> ---
> 
> Creating this Review request on Ere Maijala's patch. See SOLR-11982 for 
> previous discussion.
> It would be nice to have the possibility to easily sort the shards in the 
> preferred order e.g. by replica type. The attached patch adds support for 
> shards.sort parameter that allows one to sort e.g. PULL and TLOG replicas 
> first with ``shards.sor=replicaType:PULL|TLOG``(which would mean that NRT 
> replicas wouldn't be hit with queries unless they're the only ones available) 
> and/or to sort by replica location (like preferLocalShards=true but more 
> versatile).
> 
> 
> Diffs
> -
> 
>   solr/CHANGES.txt c1b5161c9c 
>   
> solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
>  6bfd36af94 
>   
> solr/core/src/test/org/apache/solr/handler/component/HttpShardHandlerFactoryTest.java
>  PRE-CREATION 
>   solr/solr-ref-guide/src/distributed-requests.adoc f5aaff469e 
>   solr/solrj/src/java/org/apache/solr/common/params/ShardParams.java 
> cbc33f41f4 
>   
> solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
>  9539846f88 
> 
> 
> Diff: https://reviews.apache.org/r/65942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomás Fernández Löbbe
> 
>

Re: Review Request 65942: SOLR-11982: Add support for shards.sort parameter

[Tomás Fernández Löbbe]

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




solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Lines 309 (patched)


I'm debating with myself on whether we should allow both params to be 
specified or not. I think it would be better to throw an exception in case of 
both parameters specified (since preferLocalShards would now be deprecated)



solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Line 364 (original), 325 (patched)


I think we should surround this with a try/catch and throw a 
SolrException(SolrError.BAD_REQUEST) in case of an IllegalArgumentException



solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Lines 379 (patched)


This should be an inner class or have it's own file



solr/core/src/test/org/apache/solr/handler/component/HttpShardHandlerFactoryTest.java
Lines 31 (patched)


Note that there is a `TestHttpShardHandlerFactory`, did you intentionally 
not use that one?


- Tomás Fernández Löbbe


On March 8, 2018, 5:38 p.m., Tomás Fernández Löbbe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65942/
> ---
> 
> (Updated March 8, 2018, 5:38 p.m.)
> 
> 
> Review request for lucene.
> 
> 
> Repository: lucene-solr
> 
> 
> Description
> ---
> 
> Creating this Review request on Ere Maijala's patch. See SOLR-11982 for 
> previous discussion.
> It would be nice to have the possibility to easily sort the shards in the 
> preferred order e.g. by replica type. The attached patch adds support for 
> shards.sort parameter that allows one to sort e.g. PULL and TLOG replicas 
> first with ``shards.sor=replicaType:PULL|TLOG``(which would mean that NRT 
> replicas wouldn't be hit with queries unless they're the only ones available) 
> and/or to sort by replica location (like preferLocalShards=true but more 
> versatile).
> 
> 
> Diffs
> -
> 
>   solr/CHANGES.txt c1b5161c9c 
>   
> solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
>  6bfd36af94 
>   
> solr/core/src/test/org/apache/solr/handler/component/HttpShardHandlerFactoryTest.java
>  PRE-CREATION 
>   solr/solr-ref-guide/src/distributed-requests.adoc f5aaff469e 
>   solr/solrj/src/java/org/apache/solr/common/params/ShardParams.java 
> cbc33f41f4 
>   
> solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
>  9539846f88 
> 
> 
> Diff: https://reviews.apache.org/r/65942/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomás Fernández Löbbe
> 
>

Re: Review Request 65942: SOLR-11982: Add support for shards.sort parameter

[Tomás Fernández Löbbe]

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

(Updated March 8, 2018, 5:38 p.m.)


Review request for lucene.


Repository: lucene-solr


Description
---

Creating this Review request on Ere Maijala's patch. See SOLR-11982 for 
previous discussion.
It would be nice to have the possibility to easily sort the shards in the 
preferred order e.g. by replica type. The attached patch adds support for 
shards.sort parameter that allows one to sort e.g. PULL and TLOG replicas first 
with ``shards.sor=replicaType:PULL|TLOG``(which would mean that NRT replicas 
wouldn't be hit with queries unless they're the only ones available) and/or to 
sort by replica location (like preferLocalShards=true but more versatile).


Diffs (updated)
-

  solr/CHANGES.txt c1b5161c9c 
  
solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
 6bfd36af94 
  
solr/core/src/test/org/apache/solr/handler/component/HttpShardHandlerFactoryTest.java
 PRE-CREATION 
  solr/solr-ref-guide/src/distributed-requests.adoc f5aaff469e 
  solr/solrj/src/java/org/apache/solr/common/params/ShardParams.java cbc33f41f4 
  
solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java 
9539846f88 


Diff: https://reviews.apache.org/r/65942/diff/2/

Changes: https://reviews.apache.org/r/65942/diff/1-2/


Testing
---


Thanks,

Tomás Fernández Löbbe

Re: Review Request 65942: SOLR-11982: Add support for shards.sort parameter

[Ere Maijala]

> On maalis 7, 2018, 2:49 ap, Tomás Fernández Löbbe wrote:
> >

I believe I've addressed the review comments in the latest patch.


> On maalis 7, 2018, 2:49 ap, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Line 308 (original), 311 (patched)
> > 
> >
> > We could make this class package private and do better unit tests, 
> > should be easier to test all the possible branches, specially failure 
> > scenarios.

Done. The only thing missing from the new tests is the "replicaLocation:local" 
rule. I didn't add that since it would require way more infrastructure in the 
form of a valid request and ZK.


> On maalis 7, 2018, 2:49 ap, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Line 309 (original), 312 (patched)
> > 
> >
> > This class can be made static

Done.


> On maalis 7, 2018, 2:49 ap, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Lines 310-311 (original), 313-314 (patched)
> > 
> >
> > These two could be made final, right?

Right, done.


> On maalis 7, 2018, 2:49 ap, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Lines 327 (patched)
> > 
> >
> > You could set the size of the array to ``sortRules.size()``, right?

Done.


> On maalis 7, 2018, 2:49 ap, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Line 323 (original)
> > 
> >
> > I think we should throw an exception if the rule.name is unknown (not 
> > one of the expected values)

True. Done.


> On maalis 7, 2018, 2:49 ap, Tomás Fernández Löbbe wrote:
> > solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
> > Lines 404-406 (patched)
> > 
> >
> > I don't think this can really happen (someone comparing with the 
> > replica type directly) or am I missing something?

True. It seems it's still possible to have a situation where the list contains 
only shard addresses, so I left a type check there.


> On maalis 7, 2018, 2:49 ap, Tomás Fernández Löbbe wrote:
> > solr/solr-ref-guide/src/distributed-requests.adoc
> > Lines 149 (patched)
> > 
> >
> > ``[...]replicas in the given order of precedence``. Maybe add "within 
> > each shard"? Not sure if that's clear enough. WDYT?

Agreed. I added that.


- Ere


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


On maalis 7, 2018, 2:17 ap, Tomás Fernández Löbbe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65942/
> ---
> 
> (Updated maalis 7, 2018, 2:17 ap)
> 
> 
> Review request for lucene.
> 
> 
> Repository: lucene-solr
> 
> 
> Description
> ---
> 
> Creating this Review request on Ere Maijala's patch. See SOLR-11982 for 
> previous discussion.
> It would be nice to have the possibility to easily sort the shards in the 
> preferred order e.g. by replica type. The attached patch adds support for 
> shards.sort parameter that allows one to sort e.g. PULL and TLOG replicas 
> first with ``shards.sor=replicaType:PULL|TLOG``(which would mean that NRT 
> replicas wouldn't be hit with queries unless they're the only ones available) 
> and/or to sort by replica location (like preferLocalShards=true but more 
> versatile).
> 
> 
> Diffs
> -
> 
>   solr/CHANGES.txt 99e61f1d16 
>   
> solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
>  6bfd36af94 
>   solr/solr-ref-guide/src/distributed-requests.adoc f5aaff469e 
>   solr/solrj/src/java/org/apache/solr/common/params/ShardParams.java 
> cbc33f41f4 
>   
> solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
>  9539846f88 
> 
> 
> Diff: https://reviews.apache.org/r/65942/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomás Fernández Löbbe
> 
>

Re: Review Request 65942: SOLR-11982: Add support for shards.sort parameter

[Tomás Fernández Löbbe]

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




solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Line 308 (original), 311 (patched)


We could make this class package private and do better unit tests, should 
be easier to test all the possible branches, specially failure scenarios.



solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Line 309 (original), 312 (patched)


This class can be made static



solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Lines 310-311 (original), 313-314 (patched)


These two could be made final, right?



solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Lines 327 (patched)


You could set the size of the array to ``sortRules.size()``, right?



solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Lines 331 (patched)


I think in this case we should throw an IllegalArgumentException that 
becomes a BAD_REQUEST instead of logging



solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Line 323 (original)


I think we should throw an exception if the rule.name is unknown (not one 
of the expected values)



solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
Lines 404-406 (patched)


I don't think this can really happen (someone comparing with the replica 
type directly) or am I missing something?



solr/solr-ref-guide/src/distributed-requests.adoc
Lines 149 (patched)


``[...]replicas in the given order of precedence``. Maybe add "within each 
shard"? Not sure if that's clear enough. WDYT?


- Tomás Fernández Löbbe


On March 7, 2018, 2:17 a.m., Tomás Fernández Löbbe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65942/
> ---
> 
> (Updated March 7, 2018, 2:17 a.m.)
> 
> 
> Review request for lucene.
> 
> 
> Repository: lucene-solr
> 
> 
> Description
> ---
> 
> Creating this Review request on Ere Maijala's patch. See SOLR-11982 for 
> previous discussion.
> It would be nice to have the possibility to easily sort the shards in the 
> preferred order e.g. by replica type. The attached patch adds support for 
> shards.sort parameter that allows one to sort e.g. PULL and TLOG replicas 
> first with ``shards.sor=replicaType:PULL|TLOG``(which would mean that NRT 
> replicas wouldn't be hit with queries unless they're the only ones available) 
> and/or to sort by replica location (like preferLocalShards=true but more 
> versatile).
> 
> 
> Diffs
> -
> 
>   solr/CHANGES.txt 99e61f1d16 
>   
> solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java
>  6bfd36af94 
>   solr/solr-ref-guide/src/distributed-requests.adoc f5aaff469e 
>   solr/solrj/src/java/org/apache/solr/common/params/ShardParams.java 
> cbc33f41f4 
>   
> solr/solrj/src/test/org/apache/solr/client/solrj/impl/CloudSolrClientTest.java
>  9539846f88 
> 
> 
> Diff: https://reviews.apache.org/r/65942/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomás Fernández Löbbe
> 
>