tomglk commented on a change in pull request #166:
URL: https://github.com/apache/solr/pull/166#discussion_r656487847
##########
File path: solr/contrib/ltr/src/java/org/apache/solr/ltr/LTRScoringQuery.java
##########
@@ -223,6 +223,7 @@ public ModelWeight createWeight(IndexSearcher searcher,
ScoreMode scoreMode, flo
else{
createWeightsParallel(searcher, scoreMode.needsScores(), featureWeights,
features);
}
+ // TODO: potential prefetchFields computation site?
Review comment:
> can't ever happen, right? And so if it did happen then some very
strange bug or problem presumably would have arisen
I think so too. That also made me struggle yesterday as I thought about
testing.
Your arguments make sense to me. The additional complexity that e.g. the
AtomicBoolean would provide also speaks against the use of the
`keepFeaturesFinalAfterModelCreation`.
Maybe a comment could provide a bit more clarity for people who see the
`prefetchFields = Collections.unmodifiableSortedSet(prefetchFields);` and a few
lines below `prefetchFields.addAll(fields);`
Something like this b82b880
Regarding `Cloneable`:
I just experimented a bit with the `Cloneable` interface. It seems to only
do a shallow copy which could be problematic with the prefetchFields. I used
this code:
```java
public PrefetchingFieldValueFeature clone() throws
CloneNotSupportedException {
return (PrefetchingFieldValueFeature) super.clone();
}
```
and made the PrefetchingFieldValueFeature implement Cloneable
--
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.
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
