dsmiley commented on code in PR #2571:
URL: https://github.com/apache/solr/pull/2571#discussion_r1688601156
##########
solr/solrj/src/java/org/apache/solr/client/solrj/impl/CloudSolrClient.java:
##########
@@ -1213,10 +1193,6 @@ protected DocCollection getDocCollection(String
collection, Integer expectedVers
// no such collection exists
return null;
}
- if (!ref.isLazilyLoaded()) {
Review Comment:
BTW, I uncovered that this lazy check was introduced in SOLR-6521 as part of
a performance improvement relating to ensuring that concurrent cache lookups of
an expired entry don't redundantly fetch it in parallel.
> I believe ZK CSP always has its DocCollections cached in CSP since its
works with Lazy refs
I confirmed this as well in a debugger. It's because a ZkStateReader
created *for* ZK CSP doesn't watch the state of any single collection (albeit
it does watch the parent /collections path), whereas a ZkStateReader created in
ZkController (i.e. on the server) does watch individual collections for local
replicas.
Two questions upon us: do we
* (A) remove !isLazilyLoaded short-circuit. Basically all users use ZK CSP
and this doesn't trigger for that. We fixed the HTTP CSP performance
interaction here so it can be removed.
* (B) keep the short-circuit, and make HTTP CSP return a CollectionRef that
is lazy
I actually think BOTH :-)
Just (A) is tempting; it "works" and addresses the obvious perf regression.
However, note the comment "we MUST try to get a new version" and "this is a
call to ZK" some lines below. This code is written around the idea that the
potentially expensive part is down there where we hold a lock (SOLR-6521).
Code archeology here was helpful to uncover the intent. If we don't make it
lazy, we miss the point and then SOLR-6521 accomplishes nothing for the HTTP
CSP scenario. We should go as far as asserting that the ref.isLazilyLoaded and
document getCollectionRef that it _must_ return "lazy" references.
Thinking of that lock and SOLR-6521, we could disregard the lazy/non-lazy
issue and move `ClusterState.CollectionRef ref = getCollectionRef(collection);`
from before the lock to right down into the lock before calling `get()` on it.
Then it doesn't matter if it's lazy or not; the real lookup (talking to either
ZK or HTTP Solr) will be in there, which is the intent of SOLR-6521. I played
with this locally and tests pass. Lets do this?
--
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: issues-unsubscr...@solr.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
