You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "namest504 (via GitHub)" <gi...@apache.org> on 2026/10/19 04:42:16 UTC
[PR] [improve][doc] Note in CODING.md that a delayedExecutor() task cannot be taken back [pulsar]
namest504 opened a new pull request, #26988:
URL: https://github.com/apache/pulsar/pull/26988
Fixes #26987
### Motivation
`CompletableFuture.delayedExecutor()` is used as a timer in seven places (broker, common, managed-ledger, client, client-v5). A task given to it cannot be taken back: it stays in the JDK delayer's queue until its delay elapses and keeps what its lambda captured reachable until then, and cancelling a future built on it only skips the run. This came up in the review of #26672 and took a measurement to settle; `CODING.md` has a section on asynchronous programming but does not mention it.
### Modifications
One bullet in the *Asynchronous programming* section of `CODING.md` describing the retention, the trade-off against a `ScheduledExecutorService` (which drops cancelled tasks but discards queued ones on `shutdownNow()`), and what to do about it (stop flag, small delay or captured state, a clearable reference when the retention matters), pointing to the tail-read retry in `AbstractTableViewImpl` and #26672.
### Verifying this change
- [x] Make sure that the change passes the CI checks.
Documentation only.
### Does this pull request potentially affect one of the following parts:
- [ ] Dependencies
- [ ] The public API
- [ ] The schema
- [ ] The default values of configurations
- [ ] The threading model
- [ ] The binary protocol
- [ ] The REST endpoints
- [ ] The admin CLI options
- [ ] The metrics
- [ ] Anything that affects deployment
### Documentation
- [x] `doc-not-needed`
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][doc] Note in CODING.md that a delayedExecutor() task cannot be taken back [pulsar]
Posted by "namest504 (via GitHub)" <gi...@apache.org>.
namest504 commented on code in PR #26988:
URL: https://github.com/apache/pulsar/pull/26988#discussion_r4311071880
##########
CODING.md:
##########
@@ -99,6 +99,16 @@ Pulsar relies heavily on `CompletableFuture`; prefer it over `ListenableFuture`
- **`org.apache.pulsar.common.semaphore.AsyncSemaphoreImpl`** — a non-blocking semaphore with a
per-operation cost that queues callers instead of failing when the limit is reached. Preferred over
`ConcurrencyReducer` for request-driven cases that need a timeout on permit acquisition.
+- **A task given to `CompletableFuture.delayedExecutor()` cannot be taken back.** It sits in the JDK's
+ delayer queue until its delay elapses and keeps everything its lambda captured (`this`, readers,
+ maps) reachable until then; cancelling a future built on it (`runAsync(task, delayedExecutor(...))`)
+ only skips the run. A `ScheduledExecutorService` with `setRemoveOnCancelPolicy(true)` does drop a
Review Comment:
Done in 92a06b8934.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][doc] Note in CODING.md that a delayedExecutor() task cannot be taken back [pulsar]
Posted by "namest504 (via GitHub)" <gi...@apache.org>.
namest504 commented on code in PR #26988:
URL: https://github.com/apache/pulsar/pull/26988#discussion_r4311071880
##########
CODING.md:
##########
@@ -99,6 +99,16 @@ Pulsar relies heavily on `CompletableFuture`; prefer it over `ListenableFuture`
- **`org.apache.pulsar.common.semaphore.AsyncSemaphoreImpl`** — a non-blocking semaphore with a
per-operation cost that queues callers instead of failing when the limit is reached. Preferred over
`ConcurrencyReducer` for request-driven cases that need a timeout on permit acquisition.
+- **A task given to `CompletableFuture.delayedExecutor()` cannot be taken back.** It sits in the JDK's
+ delayer queue until its delay elapses and keeps everything its lambda captured (`this`, readers,
+ maps) reachable until then; cancelling a future built on it (`runAsync(task, delayedExecutor(...))`)
+ only skips the run. A `ScheduledExecutorService` with `setRemoveOnCancelPolicy(true)` does drop a
Review Comment:
You're right, the method is on `ScheduledThreadPoolExecutor` rather than on the interface. Done in 92a06b8934.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][doc] Note in CODING.md that a delayedExecutor() task cannot be taken back [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26988:
URL: https://github.com/apache/pulsar/pull/26988#discussion_r4314622649
##########
CODING.md:
##########
@@ -99,6 +99,16 @@ Pulsar relies heavily on `CompletableFuture`; prefer it over `ListenableFuture`
- **`org.apache.pulsar.common.semaphore.AsyncSemaphoreImpl`** — a non-blocking semaphore with a
per-operation cost that queues callers instead of failing when the limit is reached. Preferred over
`ConcurrencyReducer` for request-driven cases that need a timeout on permit acquisition.
+- **A task given to `CompletableFuture.delayedExecutor()` cannot be taken back.** It sits in the JDK's
+ delayer queue until its delay elapses and keeps everything its lambda captured (`this`, readers,
+ maps) reachable until then; cancelling a future built on it (`runAsync(task, delayedExecutor(...))`)
+ only skips the run. A `ScheduledThreadPoolExecutor` configured with `setRemoveOnCancelPolicy(true)`
+ does drop a cancelled task, but a task queued there is discarded without notice by `shutdownNow()`,
+ so neither is right for every timer. When using the delayer: check a stop flag when the task runs,
+ keep the delay or the captured state small, and when the retention matters (a closed object with a
+ large state behind a long delay), capture a reference that the stop can clear instead of the object
+ itself. See the tail-read retry in `AbstractTableViewImpl` and the discussion in
+ [#26672](https://github.com/apache/pulsar/pull/26672).
Review Comment:
It's better not to reference PRs in CODING.md that agents read since it pulls in irrelevant context.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][doc] Note in CODING.md that a delayedExecutor() task cannot be taken back [pulsar]
Posted by "namest504 (via GitHub)" <gi...@apache.org>.
namest504 commented on code in PR #26988:
URL: https://github.com/apache/pulsar/pull/26988#discussion_r4320076820
##########
CODING.md:
##########
@@ -99,6 +99,16 @@ Pulsar relies heavily on `CompletableFuture`; prefer it over `ListenableFuture`
- **`org.apache.pulsar.common.semaphore.AsyncSemaphoreImpl`** — a non-blocking semaphore with a
per-operation cost that queues callers instead of failing when the limit is reached. Preferred over
`ConcurrencyReducer` for request-driven cases that need a timeout on permit acquisition.
+- **A task given to `CompletableFuture.delayedExecutor()` cannot be taken back.** It sits in the JDK's
+ delayer queue until its delay elapses and keeps everything its lambda captured (`this`, readers,
+ maps) reachable until then; cancelling a future built on it (`runAsync(task, delayedExecutor(...))`)
+ only skips the run. A `ScheduledThreadPoolExecutor` configured with `setRemoveOnCancelPolicy(true)`
+ does drop a cancelled task, but a task queued there is discarded without notice by `shutdownNow()`,
+ so neither is right for every timer. When using the delayer: check a stop flag when the task runs,
+ keep the delay or the captured state small, and when the retention matters (a closed object with a
+ large state behind a long delay), capture a reference that the stop can clear instead of the object
+ itself. See the tail-read retry in `AbstractTableViewImpl` and the discussion in
+ [#26672](https://github.com/apache/pulsar/pull/26672).
Review Comment:
I see. Dropped the example sentence altogether in 8a93d909e9; the bullet stands on its own.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][doc] Note in CODING.md that a delayedExecutor() task cannot be taken back [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #26988:
URL: https://github.com/apache/pulsar/pull/26988
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][doc] Note in CODING.md that a delayedExecutor() task cannot be taken back [pulsar]
Posted by "codelipenghui (via GitHub)" <gi...@apache.org>.
codelipenghui commented on code in PR #26988:
URL: https://github.com/apache/pulsar/pull/26988#discussion_r4311010803
##########
CODING.md:
##########
@@ -99,6 +99,16 @@ Pulsar relies heavily on `CompletableFuture`; prefer it over `ListenableFuture`
- **`org.apache.pulsar.common.semaphore.AsyncSemaphoreImpl`** — a non-blocking semaphore with a
per-operation cost that queues callers instead of failing when the limit is reached. Preferred over
`ConcurrencyReducer` for request-driven cases that need a timeout on permit acquisition.
+- **A task given to `CompletableFuture.delayedExecutor()` cannot be taken back.** It sits in the JDK's
+ delayer queue until its delay elapses and keeps everything its lambda captured (`this`, readers,
+ maps) reachable until then; cancelling a future built on it (`runAsync(task, delayedExecutor(...))`)
+ only skips the run. A `ScheduledExecutorService` with `setRemoveOnCancelPolicy(true)` does drop a
Review Comment:
Could this name `ScheduledThreadPoolExecutor`? `setRemoveOnCancelPolicy` is specific to that implementation, not `ScheduledExecutorService`, so contributors need the concrete type to enable this policy. Suggested wording: “A `ScheduledThreadPoolExecutor` configured with `setRemoveOnCancelPolicy(true)`”. Non-blocking.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [improve][doc] Note in CODING.md that a delayedExecutor() task cannot be taken back [pulsar]
Posted by "namest504 (via GitHub)" <gi...@apache.org>.
namest504 commented on code in PR #26988:
URL: https://github.com/apache/pulsar/pull/26988#discussion_r4320076820
##########
CODING.md:
##########
@@ -99,6 +99,16 @@ Pulsar relies heavily on `CompletableFuture`; prefer it over `ListenableFuture`
- **`org.apache.pulsar.common.semaphore.AsyncSemaphoreImpl`** — a non-blocking semaphore with a
per-operation cost that queues callers instead of failing when the limit is reached. Preferred over
`ConcurrencyReducer` for request-driven cases that need a timeout on permit acquisition.
+- **A task given to `CompletableFuture.delayedExecutor()` cannot be taken back.** It sits in the JDK's
+ delayer queue until its delay elapses and keeps everything its lambda captured (`this`, readers,
+ maps) reachable until then; cancelling a future built on it (`runAsync(task, delayedExecutor(...))`)
+ only skips the run. A `ScheduledThreadPoolExecutor` configured with `setRemoveOnCancelPolicy(true)`
+ does drop a cancelled task, but a task queued there is discarded without notice by `shutdownNow()`,
+ so neither is right for every timer. When using the delayer: check a stop flag when the task runs,
+ keep the delay or the captured state small, and when the retention matters (a closed object with a
+ large state behind a long delay), capture a reference that the stop can clear instead of the object
+ itself. See the tail-read retry in `AbstractTableViewImpl` and the discussion in
+ [#26672](https://github.com/apache/pulsar/pull/26672).
Review Comment:
I see. Removed the PR link in a588c98626; the sentence now points at `AbstractTableViewImpl` only.
--
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@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org