patsonluk commented on code in PR #1242:
URL: https://github.com/apache/solr/pull/1242#discussion_r1054663494
##########
solr/solrj/src/java/org/apache/solr/common/cloud/ClusterState.java:
##########
@@ -261,9 +259,10 @@ private static DocCollection collectionFromObjects(
if (log.isDebugEnabled()) {
log.debug("a collection {} has per-replica state", name);
}
- // this collection has replica states stored outside
- ReplicaStatesProvider rsp = REPLICASTATES_PROVIDER.get();
- if (rsp instanceof StatesProvider) ((StatesProvider)
rsp).isPerReplicaState = true;
+ } else {
+ // prior to this call, PRS provider is set. We should unset it before
+ // deserializing the replicas and slices
+ DocCollection.clearReplicaStateProvider();
Review Comment:
To my understand this is required as otherwise the Provider might interfere
and overrides the input values here?
I agree that using ThreadLocals could avoid modification of method
signatures as you pointed out, but I also share similar concern as @hiteshk25
that it's a bit hard to track code flow with ThreadLocal as it requires
"internal knowledge" of the code in order to know where things get
added/modified. (since the method signature/contract no longer suggest the
"full input", and we might start doing fetching at places that used to be only
assigning fields locally etc)
This invocation of `clearReplicateStateProvider` could be one of the places
that could be hard for devs that are not familiar with the ThreadLocal to
understand.
I do understand the goal of this PR is NOT the removal of threadlocal usage
😊 , it would be nice though to consider other designs as a replacement of
Threadlocal (in the future!). That could include bigger refactoring
(subclassing ClusterState that includes PRS, or adding overloading method etc).
For the moment, more comments like these to explain the rational could be
very helpful (which this comment has already done a pretty good job, but
perhaps it could also mention how the threadlocal PRS provider could override
the values if not cleared ?)
--
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
