This is an automated email from the ASF dual-hosted git repository.
adutra pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/polaris.git
The following commit(s) were added to refs/heads/main by this push:
new 94466e68b Synchronize on PolarisConfiguration.registerConfiguration()
(#3755)
94466e68b is described below
commit 94466e68bf55534a8107e9db80df4445af167a39
Author: Alexandre Dutra <[email protected]>
AuthorDate: Thu Feb 19 13:21:00 2026 +0100
Synchronize on PolarisConfiguration.registerConfiguration() (#3755)
The `ALL_CONFIGURATIONS` field is using `ArrayList` which is not
thread-safe.
The field is only accessed via `registerConfiguration()`, and all
configurations are currently registered during static initialization.
But we have two subclasses, `FeatureConfiguration` and
`BehaviorChangeConfiguration`, that both call that method.
Consider this scenario:
1. Thread A triggers loading of `FeatureConfiguration`;
2. Thread B triggers loading of `BehaviorChangeConfiguration`;
3. Both classes' static initializers run in parallel;
4. Both call `registerConfiguration()` on the shared list concurrently.
In order to avoid this, this PR changes the internal collections in
`PolarisConfiguration` to be concurrent collections, and also moves the
`AllowListHolder` component to `PolarisConfiguration`.
---
.../polaris/core/config/PolarisConfiguration.java | 52 ++++++++++++----------
.../polaris/service/config/ReservedProperties.java | 21 +--------
2 files changed, 29 insertions(+), 44 deletions(-)
diff --git
a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java
b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java
index b31555c8c..7b7090687 100644
---
a/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java
+++
b/polaris-core/src/main/java/org/apache/polaris/core/config/PolarisConfiguration.java
@@ -21,10 +21,12 @@ package org.apache.polaris.core.config;
import jakarta.annotation.Nonnull;
import java.util.ArrayList;
import java.util.List;
+import java.util.Map;
import java.util.Optional;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentSkipListSet;
import java.util.function.Function;
-import java.util.stream.Collectors;
-import java.util.stream.Stream;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -38,7 +40,9 @@ public abstract class PolarisConfiguration<T> {
private static final Logger LOGGER =
LoggerFactory.getLogger(PolarisConfiguration.class);
- private static final List<PolarisConfiguration<?>> allConfigurations = new
ArrayList<>();
+ private static final Map<String, PolarisConfiguration<?>> ALL_CONFIGURATIONS
=
+ new ConcurrentHashMap<>();
+ private static final Set<String> ALL_CATALOG_CONFIGS = new
ConcurrentSkipListSet<>();
private final String key;
private final String description;
@@ -55,33 +59,33 @@ public abstract class PolarisConfiguration<T> {
* configs.
*/
private static void registerConfiguration(PolarisConfiguration<?>
configuration) {
- for (PolarisConfiguration<?> existingConfiguration : allConfigurations) {
- if (existingConfiguration.key.equals(configuration.key)) {
+ if (ALL_CONFIGURATIONS.putIfAbsent(configuration.key(), configuration) !=
null) {
+ throw new IllegalArgumentException(
+ String.format("Config '%s' is already in use", configuration.key));
+ }
+ if (configuration.hasCatalogConfig()) {
+ if (!ALL_CATALOG_CONFIGS.add(configuration.catalogConfig())) {
throw new IllegalArgumentException(
- String.format("Config '%s' is already in use", configuration.key));
- } else {
- var configs =
- Stream.of(
- configuration.catalogConfigImpl,
- configuration.catalogConfigUnsafeImpl,
- existingConfiguration.catalogConfigImpl,
- existingConfiguration.catalogConfigUnsafeImpl)
- .flatMap(Optional::stream)
- .collect(Collectors.groupingBy(Function.identity(),
Collectors.counting()));
- for (var entry : configs.entrySet()) {
- if (entry.getValue() > 1) {
- throw new IllegalArgumentException(
- String.format("Catalog config %s is already in use",
entry.getKey()));
- }
- }
+ String.format("Catalog config '%s' is already in use",
configuration.catalogConfig()));
+ }
+ }
+ if (configuration.hasCatalogConfigUnsafe()) {
+ if (!ALL_CATALOG_CONFIGS.add(configuration.catalogConfigUnsafe())) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Catalog config '%s' is already in use",
configuration.catalogConfigUnsafe()));
}
}
- allConfigurations.add(configuration);
}
- /** Returns a list of all PolarisConfigurations that have been registered */
+ /** Returns a list of all PolarisConfigurations that have been registered. */
public static List<PolarisConfiguration<?>> getAllConfigurations() {
- return List.copyOf(allConfigurations);
+ return List.copyOf(ALL_CONFIGURATIONS.values());
+ }
+
+ /** Returns a set of all catalog config keys that have been registered. */
+ public static Set<String> getAllCatalogConfigs() {
+ return Set.copyOf(ALL_CATALOG_CONFIGS);
}
@SuppressWarnings("unchecked")
diff --git
a/runtime/service/src/main/java/org/apache/polaris/service/config/ReservedProperties.java
b/runtime/service/src/main/java/org/apache/polaris/service/config/ReservedProperties.java
index 36d971a86..09a335fe0 100644
---
a/runtime/service/src/main/java/org/apache/polaris/service/config/ReservedProperties.java
+++
b/runtime/service/src/main/java/org/apache/polaris/service/config/ReservedProperties.java
@@ -67,7 +67,7 @@ public interface ReservedProperties {
* prefix
*/
default Set<String> allowlist() {
- return AllowlistHolder.INSTANCE;
+ return PolarisConfiguration.getAllCatalogConfigs();
}
/** If true, attempts to modify a reserved property should throw an
exception. */
@@ -155,23 +155,4 @@ public interface ReservedProperties {
default -> update;
};
}
-
- class AllowlistHolder {
- static final Set<String> INSTANCE = computeAllowlist();
-
- private static Set<String> computeAllowlist() {
- Set<String> allowlist = new HashSet<>();
- PolarisConfiguration.getAllConfigurations()
- .forEach(
- c -> {
- if (c.hasCatalogConfig()) {
- allowlist.add(c.catalogConfig());
- }
- if (c.hasCatalogConfigUnsafe()) {
- allowlist.add(c.catalogConfigUnsafe());
- }
- });
- return allowlist;
- }
- }
}
