[
https://issues.apache.org/jira/browse/SOLR-12699?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16599231#comment-16599231
]
Christine Poerschke commented on SOLR-12699:
--------------------------------------------
Thanks [~slivotov] for attaching your patch here!
{quote}... in theory, this object is still mutable because ... Only in theory,
by the flow they are immutable objects ... to go further and make them really
immutable but it will require as to do a lot of changes ...
{quote}
Good point re: mutable in theory vs. immutable in practice based on code flow.
Given how the objects are constructed (including the setter invocations) and
stored and used, I think it's okay to go with your
{{Collections.unmodifiable...(...)}} approach.
Semi-related to the "is it really really immutable?" question, I've gone and
added comments for derived classes' members to clarify why they do not
influence the hash code that would now then be cached by the base class. Hope
that makes sense?
Running {{ant precommit}} which includes {{ant check-forbidden-apis}} flagged
up this
{code:java}
[forbidden-apis] Scanning classes for violations...
[forbidden-apis] Forbidden class/interface use: com.google.common.base.Supplier
[Use corresponding Java 8 functional/streaming interfaces]
[forbidden-apis] in org.apache.solr.ltr.model.LTRScoringModel
(LTRScoringModel.java, field declaration of 'cachedHashCodeSupplier')
[forbidden-apis] Forbidden class/interface use: com.google.common.base.Supplier
[Use corresponding Java 8 functional/streaming interfaces]
[forbidden-apis] in org.apache.solr.ltr.model.LTRScoringModel
(LTRScoringModel.java:174)
{code}
and in the revised patch I just attached a plain {{int hashCode}} computed in
the constructor is used instead. How does that sound?
Last but not least, *a question of backwards compatibility* i.e. what if
someone has implemented their own {{LTRScoringModel}} class which requires
mutable members? I'm struggling to imagine how and why one might wish to modify
the members after construction and it is perhaps also not entirely clear if
making non-public final members immutable is a break in backwards
compatibility. *Would love to hear your and others thoughts on this.*
* The attached patch has the addition of an interim i.e. deprecated protected
constructor – this strictly speaking would still constitute a backcompat break
though.
* An alternative could be to introduce a new {{LTRScoringModel}} constructor
alternative and deriving classes must 'opt in' to the new reduced mutability by
passing a non-null version argument.
{code:java}
+ @deprecated
public LTRScoringModel(String name, List<Feature> features,
List<Normalizer> norms,
String featureStoreName, List<Feature> allFeatures,
Map<String,Object> params) {
+ this(name, features, norms, featureStoreName, allFeatures, params, null);
+ }
+
+ public LTRScoringModel(String name, List<Feature> features,
+ List<Normalizer> norms,
+ String featureStoreName, List<Feature> allFeatures,
+ Map<String,Object> params, Integer version) {
{code}
> make LTRScoringModel immutable (to allow hashCode caching)
> ----------------------------------------------------------
>
> Key: SOLR-12699
> URL: https://issues.apache.org/jira/browse/SOLR-12699
> Project: Solr
> Issue Type: Sub-task
> Security Level: Public(Default Security Level. Issues are Public)
> Components: contrib - LTR
> Reporter: Stanislav Livotov
> Priority: Major
> Attachments: SOLR-12699.patch, SOLR-12699.patch
>
>
> [~slivotov] wrote in SOLR-12688:
> bq. ... LTRScoringModel was a mutable object. It was leading to the
> calculation of hashcode on each query, which in turn can consume a lot of
> time ... So I decided to make LTRScoringModel immutable and cache hashCode
> calculation. ...
> (Please see SOLR-12688 description for overall context and analysis results.)
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
