Re: [PR] PersistentTTLNode Thread leak [curator]
kezhuw commented on PR #1264: URL: https://github.com/apache/curator/pull/1264#issuecomment-2818120880 Merged. @chevaris Thank you for your contribution! -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
kezhuw merged PR #1264: URL: https://github.com/apache/curator/pull/1264 -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on PR #1264: URL: https://github.com/apache/curator/pull/1264#issuecomment-2817734403 I see that zk38 is failing BUT is not related with the changes. TesPersistentTtlNode are passing properly. Not sure how to proceed -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on PR #1264: URL: https://github.com/apache/curator/pull/1264#issuecomment-2815677623 I will provide a fix. Thanks for the heads up -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
kezhuw commented on PR #1264: URL: https://github.com/apache/curator/pull/1264#issuecomment-2814503805 Seems that the test fails due to [ZOOKEEPER-4466](https://issues.apache.org/jira/browse/ZOOKEEPER-4466) which is only shipped in 3.9. Both `PersistentNode` and `PersistentWatcher`(from test code) set watches(one exist and persistent_recursive) on "/parent/main". I think we probably should refactor test code to watch on "/parent", otherwise we might have to excludes these two tests from zk38 and zk37. For the same sake, I think `testTouchNodeNotCreated` is probably fragile to timing. -- 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-unsubscr...@curator.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2047192585
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java:
##
@@ -237,4 +241,88 @@ void touch() {
}
}
}
+
+@Test
+public void testInternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
+final String touchPath = ZKPaths.makePath(mainPath,
PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
+final CountDownLatch touchCreatedLatch = new CountDownLatch(1);
+try (CuratorFramework client =
+CuratorFrameworkFactory.newClient(server.getConnectString(),
new RetryOneTime(1))) {
+client.start();
+assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
+try (PersistentWatcher watcher = new PersistentWatcher(client,
mainPath, true)) {
+final Watcher listener = event -> {
+final String path = event.getPath();
+if (touchPath.equals(path)) {
+touchCreatedLatch.countDown();
+}
+};
+watcher.getListenable().addListener(listener);
+watcher.start();
+final AtomicLong executorThreadId = new AtomicLong();
+try (PersistentTtlNode node = new PersistentTtlNode(client,
mainPath, ttlMs, new byte[0])) {
+node.start();
+assertTrue(touchCreatedLatch.await(5 * ttlMs,
TimeUnit.MILLISECONDS));
+node.getCloseableScheduledExecutorService()
+.submit(() ->
+
executorThreadId.set(Thread.currentThread().getId()));
+assertFalse(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+Thread.sleep(10L);
Review Comment:
I realized (doing the code) that awaitTermination requires that
shutdown/shutdownNow has been previously invoked , otherwise will NOT work as
expected . So when executor is provided, after PersistentTTLNode has been
closed I will wait for 100ms and check that executor there is still there
Maybe looking for the thread using the threadId and the name is a bit too
much, when it is enough to check if the executor isShutdown or NOT.
I will provide commit that is able to check if excutor is shutdown in both
cases, and also the thread checks and you provide comments. Now I am more in
favour to remove all the code that looks for the thread from stacks traces, BUT
just give me your opinion there
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
kezhuw commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2046406661
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java:
##
@@ -237,4 +241,88 @@ void touch() {
}
}
}
+
+@Test
+public void testInternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
+final String touchPath = ZKPaths.makePath(mainPath,
PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
+final CountDownLatch touchCreatedLatch = new CountDownLatch(1);
+try (CuratorFramework client =
+CuratorFrameworkFactory.newClient(server.getConnectString(),
new RetryOneTime(1))) {
+client.start();
+assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
+try (PersistentWatcher watcher = new PersistentWatcher(client,
mainPath, true)) {
+final Watcher listener = event -> {
+final String path = event.getPath();
+if (touchPath.equals(path)) {
+touchCreatedLatch.countDown();
+}
+};
+watcher.getListenable().addListener(listener);
+watcher.start();
+final AtomicLong executorThreadId = new AtomicLong();
+try (PersistentTtlNode node = new PersistentTtlNode(client,
mainPath, ttlMs, new byte[0])) {
+node.start();
+assertTrue(touchCreatedLatch.await(5 * ttlMs,
TimeUnit.MILLISECONDS));
+node.getCloseableScheduledExecutorService()
+.submit(() ->
+
executorThreadId.set(Thread.currentThread().getId()));
+assertFalse(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+Thread.sleep(10L);
Review Comment:
Both sound good to me!
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2046278696
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java:
##
@@ -237,4 +241,88 @@ void touch() {
}
}
}
+
+@Test
+public void testInternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
+final String touchPath = ZKPaths.makePath(mainPath,
PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
+final CountDownLatch touchCreatedLatch = new CountDownLatch(1);
+try (CuratorFramework client =
+CuratorFrameworkFactory.newClient(server.getConnectString(),
new RetryOneTime(1))) {
+client.start();
+assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
+try (PersistentWatcher watcher = new PersistentWatcher(client,
mainPath, true)) {
+final Watcher listener = event -> {
+final String path = event.getPath();
+if (touchPath.equals(path)) {
+touchCreatedLatch.countDown();
+}
+};
+watcher.getListenable().addListener(listener);
+watcher.start();
+final AtomicLong executorThreadId = new AtomicLong();
+try (PersistentTtlNode node = new PersistentTtlNode(client,
mainPath, ttlMs, new byte[0])) {
+node.start();
+assertTrue(touchCreatedLatch.await(5 * ttlMs,
TimeUnit.MILLISECONDS));
+node.getCloseableScheduledExecutorService()
+.submit(() ->
+
executorThreadId.set(Thread.currentThread().getId()));
+assertFalse(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+Thread.sleep(10L);
Review Comment:
I have my doubts here what is best, because awaitTermination assures you
that tasks were executed, BUT the Javadoc does NOT confirm that the thread has
been destroyed, so we can only rely that will happen eventually
After closing the PersistentTTLNode:
- When executor is internal, probably best is a loop with small sleeps to
check that thread disappears. Notice also that CloseExecutorService is in
another package and extracting methods for testing requires them to be public
that I really do NOT like.
- When executor is provided, probably the best is to use awaitTermination
and after that sleep for instance 100 msecs and check that the thread is still
there
What do you think?
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2046275893
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java:
##
@@ -237,4 +241,88 @@ void touch() {
}
}
}
+
+@Test
+public void testInternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
+final String touchPath = ZKPaths.makePath(mainPath,
PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
+final CountDownLatch touchCreatedLatch = new CountDownLatch(1);
+try (CuratorFramework client =
+CuratorFrameworkFactory.newClient(server.getConnectString(),
new RetryOneTime(1))) {
+client.start();
+assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
+try (PersistentWatcher watcher = new PersistentWatcher(client,
mainPath, true)) {
+final Watcher listener = event -> {
+final String path = event.getPath();
+if (touchPath.equals(path)) {
+touchCreatedLatch.countDown();
+}
+};
+watcher.getListenable().addListener(listener);
+watcher.start();
+final AtomicLong executorThreadId = new AtomicLong();
+try (PersistentTtlNode node = new PersistentTtlNode(client,
mainPath, ttlMs, new byte[0])) {
+node.start();
+assertTrue(touchCreatedLatch.await(5 * ttlMs,
TimeUnit.MILLISECONDS));
+node.getCloseableScheduledExecutorService()
+.submit(() ->
+
executorThreadId.set(Thread.currentThread().getId()));
+assertFalse(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+Thread.sleep(10L);
+assertTrue(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+}
+}
+
+private List getThreadsWithIdAndName(final AtomicLong
executorThreadId, final String name) {
+return Thread.getAllStackTraces().keySet().stream()
+.filter(t -> t.getId() == executorThreadId.get() &&
t.getName().contains(name))
+.collect(Collectors.toList());
+}
+
+@Test
+public void testExternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
+final String touchPath = ZKPaths.makePath(mainPath,
PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
+final CountDownLatch touchCreatedLatch = new CountDownLatch(1);
+try (CuratorFramework client =
+CuratorFrameworkFactory.newClient(server.getConnectString(),
new RetryOneTime(1))) {
+client.start();
+assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
+try (PersistentWatcher watcher = new PersistentWatcher(client,
mainPath, true)) {
+final Watcher listener = event -> {
+final String path = event.getPath();
+if (touchPath.equals(path)) {
+touchCreatedLatch.countDown();
+}
+};
+watcher.getListenable().addListener(listener);
+watcher.start();
+final AtomicLong executorThreadId = new AtomicLong();
+final String threadName = "testThreadName";
+try (PersistentTtlNode node = new PersistentTtlNode(
+client,
+Executors.newSingleThreadScheduledExecutor(task -> new
Thread(task, threadName)),
Review Comment:
You are right. That was missing
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2046251680
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java:
##
@@ -237,4 +241,88 @@ void touch() {
}
}
}
+
+@Test
+public void testInternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
+final String touchPath = ZKPaths.makePath(mainPath,
PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
+final CountDownLatch touchCreatedLatch = new CountDownLatch(1);
+try (CuratorFramework client =
+CuratorFrameworkFactory.newClient(server.getConnectString(),
new RetryOneTime(1))) {
+client.start();
+assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
+try (PersistentWatcher watcher = new PersistentWatcher(client,
mainPath, true)) {
+final Watcher listener = event -> {
+final String path = event.getPath();
+if (touchPath.equals(path)) {
+touchCreatedLatch.countDown();
+}
+};
+watcher.getListenable().addListener(listener);
+watcher.start();
+final AtomicLong executorThreadId = new AtomicLong();
+try (PersistentTtlNode node = new PersistentTtlNode(client,
mainPath, ttlMs, new byte[0])) {
+node.start();
+assertTrue(touchCreatedLatch.await(5 * ttlMs,
TimeUnit.MILLISECONDS));
+node.getCloseableScheduledExecutorService()
+.submit(() ->
+
executorThreadId.set(Thread.currentThread().getId()));
+assertFalse(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+Thread.sleep(10L);
+assertTrue(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+}
+}
+
+private List getThreadsWithIdAndName(final AtomicLong
executorThreadId, final String name) {
Review Comment:
Good comment
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
kezhuw commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2045865526
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java:
##
@@ -237,4 +241,88 @@ void touch() {
}
}
}
+
+@Test
+public void testInternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
+final String touchPath = ZKPaths.makePath(mainPath,
PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
+final CountDownLatch touchCreatedLatch = new CountDownLatch(1);
+try (CuratorFramework client =
+CuratorFrameworkFactory.newClient(server.getConnectString(),
new RetryOneTime(1))) {
+client.start();
+assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
+try (PersistentWatcher watcher = new PersistentWatcher(client,
mainPath, true)) {
+final Watcher listener = event -> {
+final String path = event.getPath();
+if (touchPath.equals(path)) {
+touchCreatedLatch.countDown();
+}
+};
+watcher.getListenable().addListener(listener);
+watcher.start();
+final AtomicLong executorThreadId = new AtomicLong();
+try (PersistentTtlNode node = new PersistentTtlNode(client,
mainPath, ttlMs, new byte[0])) {
+node.start();
+assertTrue(touchCreatedLatch.await(5 * ttlMs,
TimeUnit.MILLISECONDS));
+node.getCloseableScheduledExecutorService()
+.submit(() ->
+
executorThreadId.set(Thread.currentThread().getId()));
+assertFalse(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+Thread.sleep(10L);
+assertTrue(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+}
+}
+
+private List getThreadsWithIdAndName(final AtomicLong
executorThreadId, final String name) {
Review Comment:
There will be at most one thread. And I think "assertNotNull/assertNull" is
easy to read than "assertFalse(...isEmpty())".
##
curator-recipes/src/test/java/org/apache/curator/framework/recipes/nodes/TestPersistentTtlNode.java:
##
@@ -237,4 +241,88 @@ void touch() {
}
}
}
+
+@Test
+public void testInternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
+final String touchPath = ZKPaths.makePath(mainPath,
PersistentTtlNode.DEFAULT_CHILD_NODE_NAME);
+final CountDownLatch touchCreatedLatch = new CountDownLatch(1);
+try (CuratorFramework client =
+CuratorFrameworkFactory.newClient(server.getConnectString(),
new RetryOneTime(1))) {
+client.start();
+assertTrue(client.blockUntilConnected(1, TimeUnit.SECONDS));
+try (PersistentWatcher watcher = new PersistentWatcher(client,
mainPath, true)) {
+final Watcher listener = event -> {
+final String path = event.getPath();
+if (touchPath.equals(path)) {
+touchCreatedLatch.countDown();
+}
+};
+watcher.getListenable().addListener(listener);
+watcher.start();
+final AtomicLong executorThreadId = new AtomicLong();
+try (PersistentTtlNode node = new PersistentTtlNode(client,
mainPath, ttlMs, new byte[0])) {
+node.start();
+assertTrue(touchCreatedLatch.await(5 * ttlMs,
TimeUnit.MILLISECONDS));
+node.getCloseableScheduledExecutorService()
+.submit(() ->
+
executorThreadId.set(Thread.currentThread().getId()));
+assertFalse(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+Thread.sleep(10L);
+assertTrue(getThreadsWithIdAndName(executorThreadId,
PersistentTtlNode.TOUCH_THREAD_NAME)
+.isEmpty());
+}
+}
+}
+
+private List getThreadsWithIdAndName(final AtomicLong
executorThreadId, final String name) {
+return Thread.getAllStackTraces().keySet().stream()
+.filter(t -> t.getId() == executorThreadId.get() &&
t.getName().contains(name))
+.collect(Collectors.toList());
+}
+
+@Test
+public void testExternalExecutorClose() throws Exception {
+final String mainPath = "/parent/main";
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2042797049
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentTtlNode.java:
##
@@ -61,12 +61,15 @@ public class PersistentTtlNode implements Closeable {
public static final int DEFAULT_TOUCH_SCHEDULE_FACTOR = 2;
public static final boolean DEFAULT_USE_PARENT_CREATION = true;
+private static final boolean SHUTDOWN_EXECUTOR = true;
+
private final Logger log = LoggerFactory.getLogger(getClass());
private final PersistentNode node;
private final CuratorFramework client;
private final long ttlMs;
private final int touchScheduleFactor;
private final ScheduledExecutorService executorService;
+private final boolean shutdownExecutor; // If needed to shut down
executorService when closing
Review Comment:
Included new commit.
As commented extended CloseableScheduledExecutorService , and added some
tests there.
Also fixed the recipe and added some tests (maybe redundant with
CloseableScheduledExecutorService tests
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
kezhuw commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2034443515
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentTtlNode.java:
##
@@ -61,12 +61,15 @@ public class PersistentTtlNode implements Closeable {
public static final int DEFAULT_TOUCH_SCHEDULE_FACTOR = 2;
public static final boolean DEFAULT_USE_PARENT_CREATION = true;
+private static final boolean SHUTDOWN_EXECUTOR = true;
+
private final Logger log = LoggerFactory.getLogger(getClass());
private final PersistentNode node;
private final CuratorFramework client;
private final long ttlMs;
private final int touchScheduleFactor;
private final ScheduledExecutorService executorService;
+private final boolean shutdownExecutor; // If needed to shut down
executorService when closing
Review Comment:
I saw `CloseableScheduledExecutorService`, and think it fit this use case
pretty well.
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
kezhuw commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2034733188
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentTtlNode.java:
##
@@ -61,12 +61,15 @@ public class PersistentTtlNode implements Closeable {
public static final int DEFAULT_TOUCH_SCHEDULE_FACTOR = 2;
public static final boolean DEFAULT_USE_PARENT_CREATION = true;
+private static final boolean SHUTDOWN_EXECUTOR = true;
+
private final Logger log = LoggerFactory.getLogger(getClass());
private final PersistentNode node;
private final CuratorFramework client;
private final long ttlMs;
private final int touchScheduleFactor;
private final ScheduledExecutorService executorService;
+private final boolean shutdownExecutor; // If needed to shut down
executorService when closing
Review Comment:
> Probably I could extend that as part of the solution. I assume that is OK
That would be good!
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2034683332
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentTtlNode.java:
##
@@ -61,12 +61,15 @@ public class PersistentTtlNode implements Closeable {
public static final int DEFAULT_TOUCH_SCHEDULE_FACTOR = 2;
public static final boolean DEFAULT_USE_PARENT_CREATION = true;
+private static final boolean SHUTDOWN_EXECUTOR = true;
+
private final Logger log = LoggerFactory.getLogger(getClass());
private final PersistentNode node;
private final CuratorFramework client;
private final long ttlMs;
private final int touchScheduleFactor;
private final ScheduledExecutorService executorService;
+private final boolean shutdownExecutor; // If needed to shut down
executorService when closing
Review Comment:
I think so, BUT CloseableScheduledExecutorService is missing
scheduleAtFixedDelay. Probably I could extend that as part of the solution. I
assume that is OK
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] PersistentTTLNode Thread leak [curator]
chevaris commented on code in PR #1264:
URL: https://github.com/apache/curator/pull/1264#discussion_r2034683332
##
curator-recipes/src/main/java/org/apache/curator/framework/recipes/nodes/PersistentTtlNode.java:
##
@@ -61,12 +61,15 @@ public class PersistentTtlNode implements Closeable {
public static final int DEFAULT_TOUCH_SCHEDULE_FACTOR = 2;
public static final boolean DEFAULT_USE_PARENT_CREATION = true;
+private static final boolean SHUTDOWN_EXECUTOR = true;
+
private final Logger log = LoggerFactory.getLogger(getClass());
private final PersistentNode node;
private final CuratorFramework client;
private final long ttlMs;
private final int touchScheduleFactor;
private final ScheduledExecutorService executorService;
+private final boolean shutdownExecutor; // If needed to shut down
executorService when closing
Review Comment:
I think so
--
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-unsubscr...@curator.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
