> 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) > > <https://reviews.apache.org/r/65942/diff/2/?file=1972686#file1972686line351> > > > > 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) > > <https://reviews.apache.org/r/65942/diff/2/?file=1972686#file1972686line374> > > > > 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) > > <https://reviews.apache.org/r/65942/diff/2/?file=1972686#file1972686line429> > > > > 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) > > <https://reviews.apache.org/r/65942/diff/2/?file=1972687#file1972687line31> > > > > 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 > >
