You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "FrankChen021 (via GitHub)" <gi...@apache.org> on 2026/07/30 14:46:43 UTC
[PR] Avoid sleeping while holding lifecycle locks (druid)
FrankChen021 opened a new pull request, #19810:
URL: https://github.com/apache/druid/pull/19810
## What changed
- move executor file-lock retry sleeps outside the lifecycle monitor while keeping channel creation, lock attempts, and lock publication synchronized
- move GCE compute-service retry sleeps outside the autoscaler monitor
- make the cached GCE service visible across threads and retain single initialization with a synchronized double check
## Why
Both retry loops called `Thread.sleep` while holding an object monitor. A slow file-lock acquisition or delayed GCE service creation could therefore block unrelated lifecycle or autoscaler operations and triggered CodeQL's `java/sleep-with-lock-held` rule.
## Impact
Retry timing and timeout behavior remain unchanged, but other threads no longer wait behind sleeping retry loops. Concurrent executor starts are still rejected, and the GCE compute service is still initialized at most once.
## Validation
- `ExecutorLifecycleConfigTest` (1 test)
- `GceAutoScalerTest` (12 tests)
- Maven `validate` / Checkstyle for `indexing-service` and `extensions-contrib/gce-extensions`
- `git diff --check`
Fixes CodeQL alerts 2886 and 2887.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: avoid sleeping while holding lifecycle locks (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19810:
URL: https://github.com/apache/druid/pull/19810#discussion_r3686773014
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/executor/ExecutorLifecycleTest.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.worker.executor;
+
+import org.apache.druid.indexing.common.actions.TaskActionClientFactory;
+import org.apache.druid.indexing.common.config.TaskConfig;
+import org.apache.druid.indexing.overlord.TaskRunner;
+import org.apache.druid.jackson.DefaultObjectMapper;
+import org.apache.druid.java.util.common.ISE;
+import org.joda.time.Period;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.io.File;
+import java.nio.channels.FileChannel;
+import java.nio.channels.FileLock;
+
+public class ExecutorLifecycleTest
+{
+ @Test
+ public void testAcquireTaskFileLockRetriesUntilSuccess() throws Exception
+ {
+ final ExecutorLifecycle lifecycle = createLifecycle(Period.seconds(1));
+ final FileChannel channel = Mockito.mock(FileChannel.class);
+ final FileLock lock = Mockito.mock(FileLock.class);
+ Mockito.when(channel.tryLock()).thenReturn(null, lock);
+
+ Assert.assertSame(lock, lifecycle.acquireTaskFileLock(channel, new File("task.lock")));
+ Mockito.verify(channel, Mockito.times(2)).tryLock();
Review Comment:
Thanks for the suggestion. I’m keeping the production retry implementation unchanged: this test incurs one intentional 100 ms retry wait, which is low cost, while adding an injectable sleeper or production hook solely for the test would expand the production seam and complexity. The test still directly verifies the real retry path.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: avoid sleeping while holding lifecycle locks (druid)
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #19810:
URL: https://github.com/apache/druid/pull/19810#discussion_r3686693791
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/executor/ExecutorLifecycleTest.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.worker.executor;
+
+import org.apache.druid.indexing.common.actions.TaskActionClientFactory;
+import org.apache.druid.indexing.common.config.TaskConfig;
+import org.apache.druid.indexing.overlord.TaskRunner;
+import org.apache.druid.jackson.DefaultObjectMapper;
+import org.apache.druid.java.util.common.ISE;
+import org.joda.time.Period;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.io.File;
+import java.nio.channels.FileChannel;
+import java.nio.channels.FileLock;
+
+public class ExecutorLifecycleTest
+{
+ @Test
+ public void testAcquireTaskFileLockRetriesUntilSuccess() throws Exception
+ {
+ final ExecutorLifecycle lifecycle = createLifecycle(Period.seconds(1));
+ final FileChannel channel = Mockito.mock(FileChannel.class);
+ final FileLock lock = Mockito.mock(FileLock.class);
+ Mockito.when(channel.tryLock()).thenReturn(null, lock);
+
+ Assert.assertSame(lock, lifecycle.acquireTaskFileLock(channel, new File("task.lock")));
+ Mockito.verify(channel, Mockito.times(2)).tryLock();
Review Comment:
This test will always incur a real ~100ms delay because the first `tryLock()` is stubbed to return `null`, which triggers `Thread.sleep(100)` inside `ExecutorLifecycle.acquireTaskFileLock(...)`. This slows the unit test suite and can contribute to timing flakiness.
Consider refactoring the production code to make the retry sleep overrideable/injectable (e.g., a `Sleeper`/`sleepForLockRetry()` hook) so the test can validate retry behavior without waiting on wall-clock time.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: avoid sleeping while holding lifecycle locks (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #19810:
URL: https://github.com/apache/druid/pull/19810#discussion_r3686775683
##########
indexing-service/src/test/java/org/apache/druid/indexing/worker/executor/ExecutorLifecycleTest.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.indexing.worker.executor;
+
+import org.apache.druid.indexing.common.actions.TaskActionClientFactory;
+import org.apache.druid.indexing.common.config.TaskConfig;
+import org.apache.druid.indexing.overlord.TaskRunner;
+import org.apache.druid.jackson.DefaultObjectMapper;
+import org.apache.druid.java.util.common.ISE;
+import org.joda.time.Period;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.io.File;
+import java.nio.channels.FileChannel;
+import java.nio.channels.FileLock;
+
+public class ExecutorLifecycleTest
+{
+ @Test
+ public void testAcquireTaskFileLockRetriesUntilSuccess() throws Exception
+ {
+ final ExecutorLifecycle lifecycle = createLifecycle(Period.seconds(1));
+ final FileChannel channel = Mockito.mock(FileChannel.class);
+ final FileLock lock = Mockito.mock(FileLock.class);
+ Mockito.when(channel.tryLock()).thenReturn(null, lock);
+
+ Assert.assertSame(lock, lifecycle.acquireTaskFileLock(channel, new File("task.lock")));
+ Mockito.verify(channel, Mockito.times(2)).tryLock();
Review Comment:
Thanks. This test intentionally exercises exactly one real retry interval, while the zero-timeout test covers the no-wait path. Adding a production sleep abstraction solely to avoid this 100 ms test delay would expand the production surface for little benefit. The observed CI flake was an unrelated Surefire fork/JVM-handshake failure after 20,742 successful tests; this test itself passed. Keeping the focused retry test as-is.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix: avoid sleeping while holding lifecycle locks (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 merged PR #19810:
URL: https://github.com/apache/druid/pull/19810
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org