----------------------------------------------------------- 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) <https://reviews.apache.org/r/65942/#comment278997> 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) <https://reviews.apache.org/r/65942/#comment278993> This class can be made static solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java Lines 310-311 (original), 313-314 (patched) <https://reviews.apache.org/r/65942/#comment278998> These two could be made final, right? solr/core/src/java/org/apache/solr/handler/component/HttpShardHandlerFactory.java Lines 327 (patched) <https://reviews.apache.org/r/65942/#comment278995> 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) <https://reviews.apache.org/r/65942/#comment278994> 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) <https://reviews.apache.org/r/65942/#comment278996> 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) <https://reviews.apache.org/r/65942/#comment279000> 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) <https://reviews.apache.org/r/65942/#comment279002> ``[...]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 > >
