SOLR-16427: Enable error-prone ClassInitializationDeadlock rule #2881
Conversation
There was a problem hiding this comment.
First, thanks for working on this. It's been a problem for Lucene & Solr over the years. @uschindler , you have always been involved in this topic so I'm tagging you for any additional review.
I suggest limiting this PR to be only main, without any concern for deprecating stuff. By and large, you were deprecating very internal things.
TimeSource has annoyed me; I filed SOLR-17573 to improve that (albeit may be controversial).
| qParserPlugins.init(QParserPlugin.standardPlugins, this); | ||
| valueSourceParsers.init(ValueSourceParser.standardValueSourceParsers, this); | ||
| transformerFactories.init(TransformerFactory.defaultFactories, this); | ||
| qParserPlugins.init(QParserPlugins.standardPlugins, this); |
There was a problem hiding this comment.
These 3 registries only exist for SolrCore registration. You could move them to the same package and make them not public.
But longer term (or soon if you are to do the work), I'd rather our built-in plugins be auto-discovered and registered via embracing java.util.ServiceLoader. Then we would have no static registries. -- CC @janhoy
There was a problem hiding this comment.
Some tests outside the core package currently use these registries, including:
org.apache.solr.cloud.TestRandomFlRTGCloud(ref. transformers)org.apache.solr.search.QueryEqualityTest(ref. value source parsers and query parser plugins)org.apache.solr.search.TestStandardQParsers(ref. query parser plugins)org.apache.solr.search.SignificantTermsQParserPluginTest(ref. query parser plugins)
From the logical grouping perspective it makes sense to me to keep them in the current packages. From the visibility perspective (public API) I would prefer to make them package-private / internal.
Perhaps its best to keep it as is for now until we introduce an auto-discovery feature?
solr/core/src/java/org/apache/solr/response/transform/TransformerFactory.java
Outdated
Show resolved
Hide resolved
| @@ -398,7 +398,7 @@ protected ValueSource parseValueSource(int flags) throws SyntaxError { | |||
| if ((ch >= '0' && ch <= '9') || ch == '.' || ch == '+' || ch == '-') { | |||
| Number num = sp.getNumber(); | |||
| if (num instanceof Long) { | |||
| valueSource = new ValueSourceParser.LongConstValueSource(num.longValue()); | |||
| valueSource = new ValueSourceParsers.LongConstValueSource(num.longValue()); | |||
There was a problem hiding this comment.
it appears inconsistent with the lower two lines; can you static import this? Or move to top level if the below ones are (consistency)
There was a problem hiding this comment.
This one is actually interesting. The other two value sources are from Lucene, LongConstValueSource is from Solr, which is why this is inconsistent. I could add an import, but perhaps something else could be considered here?
Would ConstValueSource be the equivalent in Lucene for long values? Because I can only find ConstValueSource for double in Lucene.
solr/core/src/java/org/apache/solr/search/ValueSourceParser.java
Outdated
Show resolved
Hide resolved
| * @deprecated Use {@link DocRouters#DEFAULT_NAME} instead. | ||
| */ | ||
| @Deprecated(since = "9.8", forRemoval = true) | ||
| public static final String DEFAULT_NAME = DocRouters.DEFAULT_NAME; |
There was a problem hiding this comment.
WDYT about keeping this as a constant string? Then almost nobody would have a need to reference the new DocRouters.
There was a problem hiding this comment.
I considered this an option, but thought that it would be better to keep DEFAULT (DocRouter) and DEFAULT_NAME (String) in the same class. So moving DEFAULT out would also affect DEFAULT_NAME.
Since I also moved out the getDocRouter(routerName) method, we still have a few more references to the new DocRouters. But perhaps that is also not necessary?
There was a problem hiding this comment.
Keeping getDocRouter(routerName) would probably be another case of class initialization deadlock, so I moved forward and removed the deprecation and kept DocRouters.DEFAULT_NAME, so that it aligns with the class reference of DocRouters.DEFAULT.
|
|
||
| import java.util.Collections; | ||
|
|
||
| public class MapWriters { |
There was a problem hiding this comment.
just for EMPTY feels sad...
Instead, I could imagine MapWriter.empty() that references an EMPTY constant declared on MapWriterMap
There was a problem hiding this comment.
Moving EMPTY to MapWriterMap and providing a static MapWriter#empty() method looks like another possible case of ClassInitializationDeadlock. Allthough it is not identified by error-prone, I believe it is similar to the examples. But maybe I am wrong on that?
|
Thanks for the review @dsmiley. I am not familiar enough with the modified classes to know which is cnosidered internal, so your input is very valuable to me. I generally try to follow the rule "anything that is public requires at least one major version of deprecation before removing it". But since it is considered internal, I will move forward and cleanup the deprecations. :) |
Note that TransformerFactories is now immutable.
…ialization-deadlock # Conflicts: # solr/solrj/src/test/org/apache/solr/client/solrj/impl/LBHttp2SolrClientIntegrationTest.java
https://issues.apache.org/jira/browse/SOLR-16427
Description
With the ClassInitializationDeadlock rule error-prone checks for possible class initialization deadlocks. See error-prone documentation for details about the problem this rule is fixing.
Solution
Following the suggestions of error-prone, the possible deadlocks are resolved by introducing separate classes for static members.
You may review commits individually for a better overview.
This PR is not backwards compatible and public static members are moved to separate classes, potentially breaking third-party code that uses these static members.
Tests
Tests were not altered, only existing references to static members are updated.
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.