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