Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
github-actions[bot] closed pull request #2881: SOLR-16427: Enable error-prone ClassInitializationDeadlock rule URL: https://github.com/apache/solr/pull/2881 -- 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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
github-actions[bot] commented on PR #2881: URL: https://github.com/apache/solr/pull/2881#issuecomment-2960840335 This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again. -- 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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
github-actions[bot] commented on PR #2881: URL: https://github.com/apache/solr/pull/2881#issuecomment-2795454928 This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! -- 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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
dsmiley commented on PR #2881: URL: https://github.com/apache/solr/pull/2881#issuecomment-2646476270 Looking back at this a bit briefly. In general these plural form classes should be very unreferenced except when the registry itself is needed (should be rare). These classes should be considered `@lucene.internal` (we don't have a Solr variation yet) as we pursue ServiceProviderInterface. Instead of `DocRouters.DEFAULT`, add `DocRouter.getDefault()`. -- 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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
uschindler commented on PR #2881:
URL: https://github.com/apache/solr/pull/2881#issuecomment-2646355888
> Thanks for your feedback @uschindler 😄
>
> > Please be still careful to not use the plural s classes during
initialization on the original class. This would reintroduce the problems.
>
> I believe this is not the case, and I hope error-prone would identify such
cases.
OK.
> > If you have accessor methods, you can use the holder pattern used in
Lucene.
>
> I haven't used this pattern before. Would
[`TimeSources.get(type)`](https://github.com/apache/solr/blob/b34abd5a5016f125174cd036cd7b3273efe6345a/solr/solrj/src/java/org/apache/solr/common/util/TimeSources.java#L49)
(previously `TimeSource.get(type)`) be such a scenario where this pattern
could be used?
The pattern can be seen here when codec initialization is done. Lucene has a
default Codec, which is a subclass of the Codec class. The Codec class has a
method `Codec#getDefault()` which returns the static default. The
initialization is delayed by a Holder pattern:
https://github.com/apache/lucene/blob/30d18dfbe1e67bed2d4550b2bc20b9b86ce9781c/lucene/core/src/java/org/apache/lucene/codecs/Codec.java#L40-L59
Here it is called:
https://github.com/apache/lucene/blob/30d18dfbe1e67bed2d4550b2bc20b9b86ce9781c/lucene/core/src/java/org/apache/lucene/codecs/Codec.java#L139-L147
Basically the trick is similar to your's but has more checks to really
prevent codec subclasses to get the default during initialization phase (that's
why the null checks are there).
> I guess in cases where the instances are public and directly accessed or
added to a list with `static { ... }` the holder pattern cannot be used?
No, that's the problem of most classes seen in this PR. It is impossible to
use that where the superclass has a static final field to one of the
subclasses. You need to break backwards compatibility.
--
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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
malliaridis commented on PR #2881:
URL: https://github.com/apache/solr/pull/2881#issuecomment-2646309035
Thanks for your feedback @uschindler 😄
> Please be still careful to not use the plural s classes during
initialization on the original class. This would reintroduce the problems.
I believe this is not the case, and I hope error-prone would identify such
cases.
> If you have accessor methods, you can use the holder pattern used in
Lucene.
I haven't used this pattern before. Would
[`TimeSources.get(type)`](https://github.com/apache/solr/blob/b34abd5a5016f125174cd036cd7b3273efe6345a/solr/solrj/src/java/org/apache/solr/common/util/TimeSources.java#L49)
(previously `TimeSource.get(type)`) be such a scenario where this pattern
could be used?
I guess in cases where the instances are public and directly accessed or
added to a list with `static { ... }` the holder pattern cannot be used?
--
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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
uschindler commented on PR #2881: URL: https://github.com/apache/solr/pull/2881#issuecomment-2646291916 Please be still careful to not use the plural s classes during initialization on the original class. This would reintroduce the problems. I wasn't able to check all those patterns, but I hope the error probe checker would gave found those, too. -- 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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
github-actions[bot] commented on PR #2881: URL: https://github.com/apache/solr/pull/2881#issuecomment-2625973468 This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the [email protected] mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution! -- 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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
malliaridis commented on code in PR #2881:
URL: https://github.com/apache/solr/pull/2881#discussion_r1860806598
##
solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java:
##
@@ -38,14 +36,24 @@
* @lucene.experimental
*/
public abstract class DocRouter {
- public static final String DEFAULT_NAME = CompositeIdRouter.NAME;
- public static final DocRouter DEFAULT;
+ /**
+ * @deprecated Use {@link DocRouters#DEFAULT_NAME} instead.
+ */
+ @Deprecated(since = "9.8", forRemoval = true)
+ public static final String DEFAULT_NAME = DocRouters.DEFAULT_NAME;
Review 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`.
--
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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
malliaridis commented on code in PR #2881: URL: https://github.com/apache/solr/pull/2881#discussion_r1860801651 ## solr/core/src/java/org/apache/solr/core/SolrCore.java: ## @@ -1128,9 +1131,9 @@ protected SolrCore( initIndex(prev != null, reload); initWriters(); - qParserPlugins.init(QParserPlugin.standardPlugins, this); - valueSourceParsers.init(ValueSourceParser.standardValueSourceParsers, this); - transformerFactories.init(TransformerFactory.defaultFactories, this); + qParserPlugins.init(QParserPlugins.standardPlugins, this); Review 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? -- 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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
malliaridis commented on code in PR #2881:
URL: https://github.com/apache/solr/pull/2881#discussion_r1860733744
##
solr/solrj/src/java/org/apache/solr/common/MapWriters.java:
##
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.common;
+
+import java.util.Collections;
+
+public class MapWriters {
Review 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?
--
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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
malliaridis commented on code in PR #2881:
URL: https://github.com/apache/solr/pull/2881#discussion_r186076
##
solr/core/src/java/org/apache/solr/search/FunctionQParser.java:
##
@@ -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());
Review 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.
--
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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
malliaridis commented on code in PR #2881:
URL: https://github.com/apache/solr/pull/2881#discussion_r1860657766
##
solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java:
##
@@ -38,14 +36,24 @@
* @lucene.experimental
*/
public abstract class DocRouter {
- public static final String DEFAULT_NAME = CompositeIdRouter.NAME;
- public static final DocRouter DEFAULT;
+ /**
+ * @deprecated Use {@link DocRouters#DEFAULT_NAME} instead.
+ */
+ @Deprecated(since = "9.8", forRemoval = true)
+ public static final String DEFAULT_NAME = DocRouters.DEFAULT_NAME;
Review 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?
--
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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
malliaridis commented on PR #2881: URL: https://github.com/apache/solr/pull/2881#issuecomment-2498408658 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. :) -- 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]
Re: [PR] SOLR-16427: Enable error-prone ClassInitializationDeadlock rule [solr]
dsmiley commented on code in PR #2881:
URL: https://github.com/apache/solr/pull/2881#discussion_r1855522803
##
solr/core/src/java/org/apache/solr/core/SolrCore.java:
##
@@ -1128,9 +1131,9 @@ protected SolrCore(
initIndex(prev != null, reload);
initWriters();
- qParserPlugins.init(QParserPlugin.standardPlugins, this);
- valueSourceParsers.init(ValueSourceParser.standardValueSourceParsers,
this);
- transformerFactories.init(TransformerFactory.defaultFactories, this);
+ qParserPlugins.init(QParserPlugins.standardPlugins, this);
Review 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
##
solr/solrj/src/java/org/apache/solr/common/MapWriters.java:
##
@@ -0,0 +1,27 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.common;
+
+import java.util.Collections;
+
+public class MapWriters {
Review Comment:
just for EMPTY feels sad...
Instead, I could imagine MapWriter.empty() that references an EMPTY constant
declared on MapWriterMap
##
solr/solrj/src/java/org/apache/solr/common/cloud/DocRouter.java:
##
@@ -38,14 +36,24 @@
* @lucene.experimental
*/
public abstract class DocRouter {
- public static final String DEFAULT_NAME = CompositeIdRouter.NAME;
- public static final DocRouter DEFAULT;
+ /**
+ * @deprecated Use {@link DocRouters#DEFAULT_NAME} instead.
+ */
+ @Deprecated(since = "9.8", forRemoval = true)
+ public static final String DEFAULT_NAME = DocRouters.DEFAULT_NAME;
Review Comment:
WDYT about keeping this as a constant string? Then almost nobody would have
a need to reference the new DocRouters.
##
solr/core/src/java/org/apache/solr/search/FunctionQParser.java:
##
@@ -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());
Review 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)
##
solr/core/src/java/org/apache/solr/search/ValueSourceParser.java:
##
@@ -128,1329 +44,19 @@ public abstract class ValueSourceParser implements
NamedListInitializedPlugin {
/** Parse the user input into a ValueSource. */
public abstract ValueSource parse(FunctionQParser fp) throws SyntaxError;
- /** standard functions supported by default, filled in static class
initialization */
- private static final Map standardVSParsers = new
HashMap<>();
-
- /** standard functions supported by default */
- public static final Map
standardValueSourceParsers =
- Collections.unmodifiableMap(standardVSParsers);
-
/**
- * Adds a new parser for the name and returns any existing one that was
overridden. This is not
- * thread safe.
+ * standard functions supported by default
+ *
+ * @deprecated Use {@link ValueSourceParsers#standardValueSourceParsers}
instead.
*/
- private static ValueSourceParser addParser(String name, ValueSourceParser p)
{
-return standardVSParsers.put(name, p);
- }
+ @Deprecated(since = "9.8", forRemoval = true)
+ public static final Map
standardValueSourceParsers =
Review Comment:
no need to deprecate
##
solr/core/src/java/org/apache/solr/search/QParserPlugin.java:
##
@@ -43,57 +32,11 @@ public abstract class QParserPlugin implements
NamedListInitializedPlugin, SolrI
* {@link QParserPlugin} has own instance of standardPlugins. This leads to
cyclic dependencies of
* static fields and to case when NAME field is not yet initialized. This
result to NPE during
* in
