cpoerschke commented on a change in pull request #166: URL: https://github.com/apache/solr/pull/166#discussion_r656440736
########## 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: > ... I never heard about `Clonable`, but that sounds like we could use it here. :) I've seen it but never used it myself as yet, so learnt something new here too :) > Yes, it makes the code more comprehensive. But my problem is, that now we just throw a generic `UnsupportedOperationException` instead of providing more information about the cause like we did with the custom message. Yes, it's a bit of a trade-off i.e. more concise code in the general common code path case at the expense of only a generic exception in the rare/impossible code path when an exception is thrown. From my current understanding of the code the ``` throw new UnsupportedOperationException("Feature " + name + " is in use by a model. Its prefetchingFields may not be changed!"); ``` can't ever happen, right? And so if it did happen then some very strange bug or problem presumably would have arisen and whilst the name of the feature might be useful to figure out the details perhaps the "is in use by a model" claim made by the exception would not be accurate in that scenario i.e. when the impossible happens very few assumptions about the state of the systems can then be made. Removal of the `keepFeaturesFinalAfterModelCreation` boolean also means that we don't need to worry about boolean vs. AtomicBoolean or other thread-safety for it and it being or not being included when the object is cloned. -- 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