> 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)
> > <https://reviews.apache.org/r/65942/diff/1/?file=1972156#file1972156line311>
> >
> >     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)
> > <https://reviews.apache.org/r/65942/diff/1/?file=1972156#file1972156line312>
> >
> >     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)
> > <https://reviews.apache.org/r/65942/diff/1/?file=1972156#file1972156line313>
> >
> >     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)
> > <https://reviews.apache.org/r/65942/diff/1/?file=1972156#file1972156line327>
> >
> >     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)
> > <https://reviews.apache.org/r/65942/diff/1/?file=1972156#file1972156line357>
> >
> >     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)
> > <https://reviews.apache.org/r/65942/diff/1/?file=1972156#file1972156line406>
> >
> >     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)
> > <https://reviews.apache.org/r/65942/diff/1/?file=1972157#file1972157line149>
> >
> >     ``[...]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
> 
>

Reply via email to