You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@lucene.apache.org by "sabi0 (via GitHub)" <gi...@apache.org> on 2024/02/07 16:18:57 UTC
[PR] Make `static final Set` constants immutable [lucene]
sabi0 opened a new pull request, #13087:
URL: https://github.com/apache/lucene/pull/13087
For the mutable "global" Sets changed `Collections.synchronizedSet()` to `ConcurrentHashMap.newKeySet()` for better performance.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #13087:
URL: https://github.com/apache/lucene/pull/13087#discussion_r1506679359
##########
lucene/backward-codecs/src/test/org/apache/lucene/backward_index/BackwardsCompatibilityTestBase.java:
##########
@@ -80,7 +81,7 @@ public abstract class BackwardsCompatibilityTestBase extends LuceneTestCase {
// make sure we never miss a version.
assertTrue("Version: " + version + " missing", binaryVersions.remove(version));
}
- BINARY_SUPPORTED_VERSIONS = binaryVersions;
+ BINARY_SUPPORTED_VERSIONS = Collections.unmodifiableSet(binaryVersions);
Review Comment:
ok all fine.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "sabi0 (via GitHub)" <gi...@apache.org>.
sabi0 commented on code in PR #13087:
URL: https://github.com/apache/lucene/pull/13087#discussion_r1506669079
##########
lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java:
##########
@@ -74,14 +73,13 @@
*/
public final class VirtualMethod<C> {
- private static final Set<Method> singletonSet =
- Collections.synchronizedSet(new HashSet<Method>());
+ private static final Set<Method> singletonSet = ConcurrentHashMap.newKeySet();
private final Class<C> baseClass;
private final String method;
private final Class<?>[] parameters;
private final ClassValue<Integer> distanceOfClass =
- new ClassValue<Integer>() {
+ new ClassValue<>() {
Review Comment:
Reverted
##########
lucene/CHANGES.txt:
##########
@@ -260,6 +260,8 @@ Improvements
* GITHUB#13092: `static final Map` constants have been made immutable (Dmitry Cherniachenko)
+* GITHUB#13087: Some `static final Set` constants were mutable - rectified. (Dmitry Cherniachenko)
Review Comment:
Done
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "sabi0 (via GitHub)" <gi...@apache.org>.
sabi0 commented on code in PR #13087:
URL: https://github.com/apache/lucene/pull/13087#discussion_r1506672577
##########
lucene/backward-codecs/src/test/org/apache/lucene/backward_index/BackwardsCompatibilityTestBase.java:
##########
@@ -80,7 +81,7 @@ public abstract class BackwardsCompatibilityTestBase extends LuceneTestCase {
// make sure we never miss a version.
assertTrue("Version: " + version + " missing", binaryVersions.remove(version));
}
- BINARY_SUPPORTED_VERSIONS = binaryVersions;
+ BINARY_SUPPORTED_VERSIONS = Collections.unmodifiableSet(binaryVersions);
Review Comment:
`TestAncientIndicesCompatibility.UNSUPPORTED_INDEXES` is already immutable:
https://github.com/apache/lucene/blob/390c109e673c9164c5359eda34223688875e4b66/lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestAncientIndicesCompatibility.java#L84
Or did you mean something else?
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on code in PR #13087:
URL: https://github.com/apache/lucene/pull/13087#discussion_r1494397690
##########
lucene/CHANGES.txt:
##########
@@ -260,6 +260,8 @@ Improvements
* GITHUB#13092: `static final Map` constants have been made immutable (Dmitry Cherniachenko)
+* GITHUB#13087: Some `static final Set` constants were mutable - rectified. (Dmitry Cherniachenko)
Review Comment:
Move to 9.11
##########
lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java:
##########
@@ -74,14 +73,13 @@
*/
public final class VirtualMethod<C> {
- private static final Set<Method> singletonSet =
- Collections.synchronizedSet(new HashSet<Method>());
+ private static final Set<Method> singletonSet = ConcurrentHashMap.newKeySet();
private final Class<C> baseClass;
private final String method;
private final Class<?>[] parameters;
private final ClassValue<Integer> distanceOfClass =
- new ClassValue<Integer>() {
+ new ClassValue<>() {
Review Comment:
this is unrelated and does not work with Java 11 (when you subclass on "new" you had to be explicit in former times). I'd like to be explicit here.
##########
lucene/backward-codecs/src/test/org/apache/lucene/backward_index/BackwardsCompatibilityTestBase.java:
##########
@@ -80,7 +81,7 @@ public abstract class BackwardsCompatibilityTestBase extends LuceneTestCase {
// make sure we never miss a version.
assertTrue("Version: " + version + " missing", binaryVersions.remove(version));
}
- BINARY_SUPPORTED_VERSIONS = binaryVersions;
+ BINARY_SUPPORTED_VERSIONS = Collections.unmodifiableSet(binaryVersions);
Review Comment:
We changed that code recently, maybe check also the other Ancient indexes test.
##########
lucene/core/src/java/org/apache/lucene/store/NativeFSLockFactory.java:
##########
@@ -65,7 +64,7 @@ public final class NativeFSLockFactory extends FSLockFactory {
/** Singleton instance */
public static final NativeFSLockFactory INSTANCE = new NativeFSLockFactory();
- private static final Set<String> LOCK_HELD = Collections.synchronizedSet(new HashSet<String>());
+ private static final Set<String> LOCK_HELD = ConcurrentHashMap.newKeySet();
Review Comment:
I think LOCK_HELP should be `Set<Path>`. This would make more sense as it would also be working with different slashes.
I know this isn't related to this cleanup task, but I just have seen it.
##########
lucene/CHANGES.txt:
##########
@@ -260,6 +260,8 @@ Improvements
* GITHUB#13092: `static final Map` constants have been made immutable (Dmitry Cherniachenko)
+* GITHUB#13087: Some `static final Set` constants were mutable - rectified. (Dmitry Cherniachenko)
Review Comment:
Also add the new set impl.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13087:
URL: https://github.com/apache/lucene/pull/13087#issuecomment-1969950639
Thanks!
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "sabi0 (via GitHub)" <gi...@apache.org>.
sabi0 commented on PR #13087:
URL: https://github.com/apache/lucene/pull/13087#issuecomment-1968530248
Hi @uschindler,
I was away from computer last week. I will take care of the conflicts and your remarks later today.
Sorry for the delay.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13087:
URL: https://github.com/apache/lucene/pull/13087#issuecomment-1952428804
I checked backwards compatibility: Except the scandinavian analyzer everything is tests or implementation private.
I think for scandinavian normalizer we need a short note in the changes, because the public set is no longer mutable. Although it would be a downstream bug to actually change it.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13087:
URL: https://github.com/apache/lucene/pull/13087#issuecomment-1968437746
Hi @sabi0,
what's going on with this? Should I merge this one after resolving the conflict? The changes entry needs to be moved to 9.11.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler commented on PR #13087:
URL: https://github.com/apache/lucene/pull/13087#issuecomment-1968559762
> Hi @uschindler, I was away from computer last week. I will take care of the conflicts and your remarks later today. Sorry for the delay.
There seems to be overlap with the latest PR regarding concurrent sets. Maybe revert those changes from this PR.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "sabi0 (via GitHub)" <gi...@apache.org>.
sabi0 commented on PR #13087:
URL: https://github.com/apache/lucene/pull/13087#issuecomment-1969921461
> There seems to be overlap with the latest PR regarding concurrent sets. Maybe revert those changes from this PR.
This was a good idea. Thank you.
Now there is a clean separation between "immutable" (this PR) and "concurrent" sets (#13142).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "dweiss (via GitHub)" <gi...@apache.org>.
dweiss commented on code in PR #13087:
URL: https://github.com/apache/lucene/pull/13087#discussion_r1486863725
##########
lucene/replicator/src/test/org/apache/lucene/replicator/nrt/TestSimpleServer.java:
##########
@@ -61,7 +61,7 @@
@SuppressForbidden(reason = "We need Unsafe to actually crush :-)")
public class TestSimpleServer extends LuceneTestCase {
- static final Set<Thread> clientThreads = Collections.synchronizedSet(new HashSet<>());
Review Comment:
Seems fine even here as it only affects one test.
##########
lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java:
##########
@@ -74,14 +73,13 @@
*/
public final class VirtualMethod<C> {
- private static final Set<Method> singletonSet =
Review Comment:
Same here, perhaps (or merge with the issue above)?
##########
lucene/core/src/java/org/apache/lucene/store/NativeFSLockFactory.java:
##########
@@ -65,7 +64,7 @@ public final class NativeFSLockFactory extends FSLockFactory {
/** Singleton instance */
public static final NativeFSLockFactory INSTANCE = new NativeFSLockFactory();
- private static final Set<String> LOCK_HELD = Collections.synchronizedSet(new HashSet<String>());
Review Comment:
There are subtle differences between these two things (synchronized set and a concurrent set). I would leave this change to a separate issue so that it can be assessed separately (I think it's a good idea, but we need to track down all the uses and make sure nothing will break).
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "sabi0 (via GitHub)" <gi...@apache.org>.
sabi0 commented on code in PR #13087:
URL: https://github.com/apache/lucene/pull/13087#discussion_r1506677807
##########
lucene/core/src/java/org/apache/lucene/store/NativeFSLockFactory.java:
##########
@@ -65,7 +64,7 @@ public final class NativeFSLockFactory extends FSLockFactory {
/** Singleton instance */
public static final NativeFSLockFactory INSTANCE = new NativeFSLockFactory();
- private static final Set<String> LOCK_HELD = Collections.synchronizedSet(new HashSet<String>());
+ private static final Set<String> LOCK_HELD = ConcurrentHashMap.newKeySet();
Review Comment:
I will open a separate PR for this.
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org
Re: [PR] Make `static final Set` constants immutable [lucene]
Posted by "uschindler (via GitHub)" <gi...@apache.org>.
uschindler merged PR #13087:
URL: https://github.com/apache/lucene/pull/13087
--
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: issues-unsubscribe@lucene.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@lucene.apache.org
For additional commands, e-mail: issues-help@lucene.apache.org