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.
This invocation of `clearReplicateStateProvider` could be one of the places
that could be hard for dev 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. That probably would include bigger changes (subclassing
ClusterState that includes PRS, or adding overloading method etc). For the
moment, more comments to explain the rational would be very helpful (which this
comment also does 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: [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]
