Copilot commented on code in PR #15862:
URL: https://github.com/apache/iceberg/pull/15862#discussion_r3033464151
##########
core/src/main/java/org/apache/iceberg/util/LockManagers.java:
##########
@@ -109,9 +111,16 @@ public int heartbeatThreads() {
}
public ScheduledExecutorService scheduler() {
- if (scheduler == null) {
+ synchronized (BaseLockManager.class) {
+ if (!registered) {
+ activeManagers += 1;
+ registered = true;
+ }
+ }
Review Comment:
BaseLockManager.scheduler() increments activeManagers/sets registered in one
synchronized block, then initializes the shared scheduler in a separate block.
This allows close() to interleave between the two and decrement activeManagers
back to 0 before the scheduler is created, leaving a live scheduler with
activeManagers==0 (and potentially never shut down). Consider doing
registration and scheduler initialization under a single
synchronized(BaseLockManager.class) block to keep the reference count
consistent under concurrent scheduler()/close() calls.
##########
core/src/test/java/org/apache/iceberg/util/TestLockManagers.java:
##########
@@ -41,6 +42,23 @@ public void testLoadCustomLockManager() {
assertThat(LockManagers.from(properties)).isInstanceOf(CustomLockManager.class);
}
+ @Test
+ public void testSchedulerUsageRegistersManagerWithoutInitialize() throws
Exception {
+ TestBaseLockManager firstManager = new TestBaseLockManager();
+ TestBaseLockManager secondManager = new TestBaseLockManager();
+
Review Comment:
This test calls BaseLockManager.scheduler() on TestBaseLockManager instances
without calling initialize(). Since BaseLockManager.heartbeatThreads defaults
to 0 until initialize runs, the first scheduler() call can attempt
Executors.newScheduledThreadPool(0) and throw IllegalArgumentException (or
become order-dependent if another test already initialized the static
scheduler). Initialize at least the manager that first creates the scheduler
(or set default values in BaseLockManager) so the test is deterministic.
```suggestion
TestBaseLockManager secondManager = new TestBaseLockManager();
Map<String, String> properties = Maps.newHashMap();
firstManager.initialize(properties);
secondManager.initialize(properties);
```
##########
core/src/test/java/org/apache/iceberg/util/TestInMemoryLockManager.java:
##########
@@ -175,4 +175,22 @@ public void testAcquireMultiProcessOnlyOneSucceed() {
.as("only 1 thread should have acquired the lock")
.isOne();
}
+
+ @Test
+ public void testClosingOneManagerDoesNotBreakAnother() throws Exception {
+ LockManagers.InMemoryLockManager anotherManager =
+ new LockManagers.InMemoryLockManager(Maps.newHashMap());
+
+ try {
+ lockManager.close();
+ assertThat(anotherManager.acquire(lockEntityId, ownerId)).isTrue();
+ assertThat(anotherManager.release(lockEntityId, ownerId)).isTrue();
Review Comment:
As written, this regression test closes lockManager before any operation
that would create/use the shared scheduler. That means it may not exercise the
original failure mode (scheduler terminated but still referenced, causing
RejectedExecutionException on scheduleAtFixedRate). To make this a real
regression, trigger scheduler creation on one manager (e.g., acquire once) and
ensure the other manager has registered/used the scheduler before closing the
first, then verify the remaining manager can still acquire without
RejectedExecutionException (or assert scheduler is not shutdown).
```suggestion
lockManager.initialize(
ImmutableMap.of(
CatalogProperties.LOCK_HEARTBEAT_INTERVAL_MS, "100",
CatalogProperties.LOCK_ACQUIRE_INTERVAL_MS, "50",
CatalogProperties.LOCK_ACQUIRE_TIMEOUT_MS, "1000"));
anotherManager.initialize(
ImmutableMap.of(
CatalogProperties.LOCK_HEARTBEAT_INTERVAL_MS, "100",
CatalogProperties.LOCK_ACQUIRE_INTERVAL_MS, "50",
CatalogProperties.LOCK_ACQUIRE_TIMEOUT_MS, "1000"));
String firstOwner = UUID.randomUUID().toString();
String secondOwner = UUID.randomUUID().toString();
String thirdOwner = UUID.randomUUID().toString();
assertThat(lockManager.acquire(lockEntityId, firstOwner)).isTrue();
assertThat(lockManager.release(lockEntityId, firstOwner)).isTrue();
assertThat(anotherManager.acquire(lockEntityId, secondOwner)).isTrue();
assertThat(anotherManager.release(lockEntityId, secondOwner)).isTrue();
lockManager.close();
assertThat(anotherManager.acquire(lockEntityId, thirdOwner)).isTrue();
assertThat(anotherManager.release(lockEntityId, thirdOwner)).isTrue();
```
--
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]
