On Tue, Apr 26, 2022 at 12:50 PM Gus Heck <[email protected]> wrote:
> I was shocked to discover https://issues.apache.org/jira/browse/SOLR-14967 > causes solr to violate one of it's most key precepts that zk is not > involved on every query. > > In looking into this, I ran across this code: > > https://github.com/apache/solr/blob/b218c177b8e3b387ada03acbad214a0c3bfe1443/solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java#L965 > That logic that checks that the params is an instanceof ModifiableSolrParams has such a smell to it that I like to think it wouldn't have passed anyone's code review. Not that there is an obvious solution (we can think of some but there are trade-offs) but the logic as it was committed is too hacky / WIP. > And this has a couple issues... > > First there is no guarantee that changes to the object returned by > getParams() will be reflected on the request, which is apparently entirely > free to return a copy of the parameters instead of the object it holds (a > number of request classes do this) or construct a new object every time the > method is called (quite a lot of request classes do this too). > Right. > Second, as it turns out the most relevant classes do in fact return a > reference rather than a copy (AbstractUpdateRequest) and QueryRequest, > though QueryRequest can hold any type of SolrParams, which might not be > modifiable... an as the comment asks... what then? > > I am not sure I can come up with a good reason that this freedom exists in > the API, and why there are so many implementations (mostly admin) where > request objects produce a new (mutable!) object every time getParams() is > called. Is there somewhere we pass request objects to secondary threads > that I'm not remembering? > > This entire area seems ripe for a (10.x) revamp... which if there's no > good answer to the above questions should maybe standardize on use of > ModifieableSolrParams by default and any subclass that really thinks it > need defensive/immutable params documenting that in it's javadoc and using > a subclass that throws an exception if an attempt to modify is made... > Something like that sounds reasonable. Maybe draft up something more concrete with an example or two. I could also imagine making ModifiableSolrParams more of a builder that *produces* an immutable SolrParams. Just an idea. Of course any talk of messing with SolrParams has vast repercussions across the codebase. We should tread carefully (get broad agreement and consider what API is breaking when). > That might be crazy talk, but I haven't yet talked myself out of it, maybe > someone here can save me some time and tell me why it's crazy? ;-) > > -Gus > > -- > http://www.needhamsoftware.com (work) > http://www.the111shift.com (play) >
