Make `static final Set` constants immutable #13087

sabi0 · 2024-02-07T16:18:54Z

For the mutable "global" Sets changed Collections.synchronizedSet() to ConcurrentHashMap.newKeySet() for better performance.

For the mutable "global" Sets changed Collections.synchronizedSet() to ConcurrentHashMap.newKeySet() for better performance.

# Conflicts: # lucene/CHANGES.txt

uschindler

This PR has 2 different types of changes: One is to use ConcurrentHashMap for some synchronized sets, the other one is making stuff final and unmodifiable.

I generally agree, but we could better communicate that in the changes log.

uschindler · 2024-02-19T11:15:48Z

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();


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.

I will open a separate PR for this.

uschindler · 2024-02-19T11:16:17Z

...ackward-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);


We changed that code recently, maybe check also the other Ancient indexes test.

TestAncientIndicesCompatibility.UNSUPPORTED_INDEXES is already immutable:

lucene/lucene/backward-codecs/src/test/org/apache/lucene/backward_index/TestAncientIndicesCompatibility.java

Line 84 in 390c109

UNSUPPORTED_INDEXES = Collections.unmodifiableSet(indices);

Or did you mean something else?

ok all fine.

uschindler · 2024-02-19T11:17:39Z

lucene/core/src/java/org/apache/lucene/util/VirtualMethod.java


  private final Class<C> baseClass;
  private final String method;
  private final Class<?>[] parameters;
  private final ClassValue<Integer> distanceOfClass =
-      new ClassValue<Integer>() {
+      new ClassValue<>() {


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.

uschindler · 2024-02-19T11:18:26Z

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)


Move to 9.11

Also add the new set impl.

uschindler · 2024-02-19T13:15:07Z

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.

uschindler · 2024-02-28T08:09:15Z

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.

sabi0 · 2024-02-28T09:05:52Z

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.

uschindler · 2024-02-28T09:23:18Z

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.

fixup! fixup! Make `static final Set` constants immutable

# Conflicts: # lucene/CHANGES.txt

sabi0 · 2024-02-28T21:09:59Z

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).

uschindler · 2024-02-28T21:28:32Z

Thanks!

dweiss

Thanks!

dweiss · 2024-02-12T22:24:01Z

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>());


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).

dweiss · 2024-02-12T22:24:59Z

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 =


Same here, perhaps (or merge with the issue above)?

dweiss · 2024-02-12T22:25:46Z

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<>());


Seems fine even here as it only affects one test.

sabi0 added 2 commits February 7, 2024 17:17

Make static final Set constants immutable

7d00c50

For the mutable "global" Sets changed Collections.synchronizedSet() to ConcurrentHashMap.newKeySet() for better performance.

Update CHANGES.txt

1b2ab16

kkmr approved these changes Feb 12, 2024

View reviewed changes

Merge branch 'main' into mutable_constants

f5ace6f

# Conflicts: # lucene/CHANGES.txt

uschindler requested changes Feb 19, 2024

View reviewed changes

uschindler self-assigned this Feb 19, 2024

uschindler added this to the 9.11.0 milestone Feb 19, 2024

sabi0 added 3 commits February 28, 2024 22:01

fixup! Make static final Set constants immutable

b7dbc2e

fixup! fixup! Make `static final Set` constants immutable

fixup! Update CHANGES.txt

ac1d292

Merge branch 'refs/heads/main' into mutable_constants

acaea13

# Conflicts: # lucene/CHANGES.txt

uschindler approved these changes Feb 28, 2024

View reviewed changes

uschindler merged commit 7b01f2f into apache:main Feb 28, 2024
4 checks passed

asfgit pushed a commit that referenced this pull request Feb 28, 2024

Make static final Set constants immutable (#13087)

091c10b

dweiss self-requested a review February 29, 2024 07:00

dweiss reviewed Feb 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `static final Set` constants immutable #13087

Make `static final Set` constants immutable #13087

sabi0 commented Feb 7, 2024

uschindler left a comment

uschindler Feb 19, 2024

sabi0 Feb 28, 2024

uschindler Feb 19, 2024

sabi0 Feb 28, 2024

uschindler Feb 28, 2024

uschindler Feb 19, 2024

sabi0 Feb 28, 2024

uschindler Feb 19, 2024

uschindler Feb 19, 2024

sabi0 Feb 28, 2024

uschindler commented Feb 19, 2024

uschindler commented Feb 28, 2024

sabi0 commented Feb 28, 2024

uschindler commented Feb 28, 2024

sabi0 commented Feb 28, 2024

uschindler commented Feb 28, 2024

dweiss left a comment

dweiss Feb 12, 2024

dweiss Feb 12, 2024

dweiss Feb 12, 2024

		@@ -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)

Make static final Set constants immutable #13087

Make static final Set constants immutable #13087

Conversation

sabi0 commented Feb 7, 2024

uschindler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uschindler commented Feb 19, 2024

uschindler commented Feb 28, 2024

sabi0 commented Feb 28, 2024

uschindler commented Feb 28, 2024

sabi0 commented Feb 28, 2024

uschindler commented Feb 28, 2024

dweiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Make `static final Set` constants immutable #13087

Make `static final Set` constants immutable #13087