Copilot commented on code in PR #15862:
URL: https://github.com/apache/iceberg/pull/15862#discussion_r3025919435
##########
core/src/main/java/org/apache/iceberg/util/LockManagers.java:
##########
@@ -155,19 +157,32 @@ public void initialize(Map<String, String> properties) {
properties,
CatalogProperties.LOCK_HEARTBEAT_THREADS,
CatalogProperties.LOCK_HEARTBEAT_THREADS_DEFAULT);
+ synchronized (BaseLockManager.class) {
+ if (!registered) {
+ activeManagers += 1;
+ registered = true;
+ }
+ }
Review Comment:
The new scheduler lifecycle depends on callers invoking `initialize(...)`
for the manager to be counted in `activeManagers`. That introduces a hidden
lifecycle requirement: managers created/used without `initialize` (e.g., direct
instantiation) won’t prevent scheduler shutdown by other managers. Consider
enforcing the contract (e.g., register in the constructor), or registering on
first scheduler use (e.g., in `scheduler()`), or at minimum documenting that
`initialize` must be called for correct shared-scheduler behavior.
##########
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();
+ lockManager = anotherManager;
+ anotherManager = null;
+ } finally {
+ if (anotherManager != null) {
+ anotherManager.close();
+ }
+ }
+ }
Review Comment:
This test constructs `anotherManager` with an empty properties map and never
calls `initialize(...)`. In the updated implementation, `BaseLockManager` only
increments `activeManagers` in `initialize`, so this test may not actually
simulate the intended scenario (‘another manager remains active’) and may not
exercise the shared scheduler path (depending on defaults). To make this
regression robust, initialize `anotherManager` with the same (or equivalent)
lock/heartbeat properties as `lockManager` and ensure the scheduler is actually
used (e.g., by performing an acquire that triggers heartbeat scheduling) before
closing the first manager.
##########
core/src/main/java/org/apache/iceberg/util/LockManagers.java:
##########
@@ -81,12 +81,14 @@ private static LockManager loadLockManager(String impl,
Map<String, String> prop
public abstract static class BaseLockManager implements LockManager {
private static volatile ScheduledExecutorService scheduler;
+ private static int activeManagers = 0;
private long acquireTimeoutMs;
private long acquireIntervalMs;
private long heartbeatIntervalMs;
private long heartbeatTimeoutMs;
private int heartbeatThreads;
+ private boolean registered = false;
Review Comment:
The new scheduler lifecycle depends on callers invoking `initialize(...)`
for the manager to be counted in `activeManagers`. That introduces a hidden
lifecycle requirement: managers created/used without `initialize` (e.g., direct
instantiation) won’t prevent scheduler shutdown by other managers. Consider
enforcing the contract (e.g., register in the constructor), or registering on
first scheduler use (e.g., in `scheduler()`), or at minimum documenting that
`initialize` must be called for correct shared-scheduler behavior.
##########
core/src/main/java/org/apache/iceberg/util/LockManagers.java:
##########
@@ -81,12 +81,14 @@ private static LockManager loadLockManager(String impl,
Map<String, String> prop
public abstract static class BaseLockManager implements LockManager {
private static volatile ScheduledExecutorService scheduler;
+ private static int activeManagers = 0;
Review Comment:
The new scheduler lifecycle depends on callers invoking `initialize(...)`
for the manager to be counted in `activeManagers`. That introduces a hidden
lifecycle requirement: managers created/used without `initialize` (e.g., direct
instantiation) won’t prevent scheduler shutdown by other managers. Consider
enforcing the contract (e.g., register in the constructor), or registering on
first scheduler use (e.g., in `scheduler()`), or at minimum documenting that
`initialize` must be called for correct shared-scheduler behavior.
##########
core/src/main/java/org/apache/iceberg/util/LockManagers.java:
##########
@@ -155,19 +157,32 @@ public void initialize(Map<String, String> properties) {
properties,
CatalogProperties.LOCK_HEARTBEAT_THREADS,
CatalogProperties.LOCK_HEARTBEAT_THREADS_DEFAULT);
+ synchronized (BaseLockManager.class) {
+ if (!registered) {
+ activeManagers += 1;
+ registered = true;
+ }
+ }
}
@Override
public void close() throws Exception {
- if (scheduler != null) {
- List<Runnable> tasks = scheduler.shutdownNow();
- tasks.forEach(
- task -> {
- if (task instanceof Future) {
- ((Future<?>) task).cancel(true);
- }
- });
- scheduler = null;
+ synchronized (BaseLockManager.class) {
+ if (registered) {
+ activeManagers -= 1;
+ registered = false;
+ }
+
+ if (activeManagers == 0 && scheduler != null) {
Review Comment:
The new scheduler lifecycle depends on callers invoking `initialize(...)`
for the manager to be counted in `activeManagers`. That introduces a hidden
lifecycle requirement: managers created/used without `initialize` (e.g., direct
instantiation) won’t prevent scheduler shutdown by other managers. Consider
enforcing the contract (e.g., register in the constructor), or registering on
first scheduler use (e.g., in `scheduler()`), or at minimum documenting that
`initialize` must be called for correct shared-scheduler behavior.
--
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]
