gerlowskija commented on code in PR #3876:
URL: https://github.com/apache/solr/pull/3876#discussion_r2576906295
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/PreemptiveBasicAuthClientCustomizer.java:
##########
@@ -66,15 +68,15 @@ public static void setDefaultSolrParams(SolrParams params) {
}
@Override
- public void close() throws IOException {}
-
- @Override
- public void setup(Http2SolrClient client) {
+ public void setup(SolrClient client) {
+ if (client instanceof Http2SolrClient == false) {
+ return;
+ }
Review Comment:
I'll try to be as specific as I can:
When I see an interface name that includes 'SolrClient' and a single method
that consumes a `SolrClient`, it implies to me that implementations will be
able to handle any old 'SolrClient' I might pass in. And that's 100% *not* the
case. Our out-of-the-box implementations are very Http2SolrClient specific,
going so far as to throw an exception if given a different type of client.
To argue from a parallel case: Java's
[List.removeAll](https://docs.oracle.com/javase/8/docs/api/java/util/List.html#removeAll-java.util.Collection-)
method consumes a "Collection" of elements to remove. Java users see that and
assume that List implementations are ambivalent to the specific type of
Collection passed in.
They'd find it very trappy if `ArrayList.removeAll` actually only supported
`SortedSet` parameters and threw an exception when anything else was passed in.
IMO that's kindof what we're doing with SolrClientCustomizer here: promising
to handle a much broader type than we actually support in practice.
> I'm ready to give up and do what you think I should do (which I'm not 100%
clear on what that is)
Don't give up - I approved the PR because I trusted your judgement and would
be happy to live with this PR landing "as is". You've done your due-diligence
in hearing me out - if I haven't convinced you, just merge it. Don't make the
change just for my sake 😛
On what I prefer: I really like the idea you sketched above. Give the
interface declaration a type parameter, so that implementations can declare
which subset of SolrClient they support (e.g. `class
PreemptiveBasicAuthClientCustomizer implements
SolrClientCustomizer<Http2SolrClient>`).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
