You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "lhotari (via GitHub)" <gi...@apache.org> on 2026/08/13 17:24:53 UTC
[PR] [fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor [pulsar]
lhotari opened a new pull request, #26327:
URL: https://github.com/apache/pulsar/pull/26327
<!--
### Contribution Checklist
- PR title format should be *[type][component] summary*. The valid `[type]` and `[component]`/scope
prefixes are enforced by CI (see `.github/workflows/ci-semantic-pull-request.yml`); for details,
see *[Guideline - Pulsar PR Naming Convention](https://pulsar.apache.org/contribute/develop-semantic-title/)*.
- Fill out the template below to describe the changes contributed by the pull request. That will give reviewers the context they need to do the review.
- The **Motivation** and **Modifications** sections are required: explain *why* (the problem/context)
and *what/how* (the change). A title alone, or a description that only restates the title, is not enough.
- Each pull request should address only one issue, not mix up code from multiple issues.
- Each commit in the pull request has a meaningful commit message
- For the local build/test/PR workflow see `CONTRIBUTING.md`, and for coding conventions see
`CODING.md`. If you use an AI coding assistant, see `AGENTS.md`.
- Once all items of the checklist are addressed, remove the above text and this checklist, leaving only the filled out template below.
-->
<!-- Either this PR fixes/closes an issue — use "Fixes #xyz" or, equivalently, "Closes #xyz", -->
Fixes #xyz
<!-- or this PR is one task of an issue -->
Main Issue: #xyz
<!-- If the PR belongs to a PIP, please add the PIP link here -->
PIP: #xyz
<!-- Details of when a PIP is required and how the PIP process work, please see: https://github.com/apache/pulsar/blob/master/pip/README.md -->
### Motivation
<!-- Explain here the context, and why you're making that change. What is the problem you're trying to solve. -->
### Modifications
<!-- Describe the modifications you've done. -->
### Verifying this change
- [ ] Make sure that the change passes the CI checks.
*(Please pick either of the following options)*
This change is a trivial rework / code cleanup without any test coverage.
*(or)*
This change is already covered by existing tests, such as *(please describe tests)*.
*(or)*
This change added tests and can be verified as follows:
*(example:)*
- *Added integration tests for end-to-end deployment with large payloads (10MB)*
- *Extended integration test for recovery after broker failure*
### Does this pull request potentially affect one of the following parts:
<!-- DO NOT REMOVE THIS SECTION. CHECK THE PROPER BOX ONLY. -->
*If the box was checked, please highlight the changes*
- [ ] Dependencies (add or upgrade a dependency)
- [ ] 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
--
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] [fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor [pulsar]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #26327:
URL: https://github.com/apache/pulsar/pull/26327#discussion_r3778723527
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java:
##########
@@ -137,6 +138,34 @@ protected CompletableFuture<Map<String, String>> computeAuthHeaders(URI uri) {
.thenApply(headers -> (headers == null || headers.isEmpty()) ? null : headers.asMap());
}
}
+ // The deprecated v4 hooks may block — an OAuth2 or Athenz shim's getAuthData() refreshes its
+ // credential with a synchronous HTTP exchange — and this method runs on whatever thread issued the
+ // admin call, which for a broker calling its own admin client is a request-handling thread.
+ // HttpClient.computeAuthHeaders off-loads the identical composition for the lookup path and names
+ // the self-deadlock it avoids; this was the last caller-thread credential resolution left, and the
+ // one place PIP-478's "every synchronous v4 plugin call is off-loaded" was still an overstatement.
+ //
+ // A null executor sends the work to the framework's shared blocking pool rather than running it
+ // inline. That is deliberate: BaseResource is constructed by 30 subclasses that would each have to
+ // thread an executor through, and the shared pool is exactly what PIP-478 provides for a component
+ // with none to lend. The admin's own bounded pool is still what a services-aware plugin gets, so
+ // the work that can actually stall — a KDC or IdP round trip inside the plugin — stays isolated
+ // per admin.
+ return V5AuthContexts.supplyBlocking(null, () -> v4AuthHeaders(uri)).thenCompose(headers -> headers);
Review Comment:
Passing `null` sends every v4 credential resolution to the process-wide eight-thread fallback pool. This is exactly the work that can stall: `AuthenticationOAuth2#getAuthData()` is synchronized and performs the blocking IdP fetch on expiry, so eight concurrent requests from one admin can occupy all shared workers (including threads waiting for that lock) and delay unrelated admins/clients. Please pass the admin-owned authentication executor into `BaseResource` and use it here so the new per-admin blast-radius isolation also covers the v4 path.
This issue also appears on line 167 of the same file.
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/PulsarAdminImpl.java:
##########
@@ -583,16 +594,46 @@ private void bindAuthenticationServices(ClientConfigurationData conf) {
() -> null, () -> null, () -> null, () -> authTlsFactory(conf), conf, clientInstanceId);
OpenTelemetry openTelemetry = conf.getOpenTelemetry() != null ? conf.getOpenTelemetry()
: OpenTelemetry.noop();
- // No scheduler: nothing on the admin path schedules periodic authentication work. The blocking
- // executor is left unbound deliberately too — a plugin that needs one falls back to the shared pool
- // rather than running on the caller thread, which is where the SASL-over-HTTP challenge rounds
- // off-load their GSSAPI work. Handing the admin's own request threads over instead would let a slow
- // KDC consume them.
+ // No scheduler: nothing on the admin path schedules periodic authentication work, and a plugin that
+ // does schedule some gets the framework's shared one — binding a scheduled pool per admin to sit idle
+ // would cost more than it buys.
+ //
+ // The blocking executor *is* the admin's own. This is where the SASL-over-HTTP challenge rounds put
+ // their GSSAPI work, so it must never be the admin's request threads — a slow KDC would consume them
+ // — but leaving it unbound is not right either: the plugin then shares one process-wide pool with
+ // every other client in the JVM, so a stalled identity provider reached by one admin throttles
+ // authentication for all of them. A small bounded pool per admin keeps that blast radius inside the
+ // admin that owns the plugin, and is shut down with it.
ClientAuthenticationServices services = new DefaultClientAuthenticationServices(
- authHttpClientFactory, null, null, Clock.systemDefaultZone(), openTelemetry, clientInstanceId);
+ authHttpClientFactory, null, blockingAuthExecutor(), Clock.systemDefaultZone(), openTelemetry,
+ clientInstanceId);
aware.bindClientAuthenticationServices(services);
}
+ /**
+ * The admin's bounded executor for potentially-blocking authentication work (PIP-478).
+ *
+ * <p>Queues rather than rejects: every caller is an authenticated request, so a saturated pool must slow
+ * requests down rather than fail them. Core threads time out, so an admin whose plugin never blocks pays
+ * for nothing. Created lazily under this admin's monitor and shut down in {@link #close()}.
+ *
+ * @return the blocking authentication executor
+ */
+ private synchronized Executor blockingAuthExecutor() {
+ if (blockingAuthExecutor == null) {
+ ThreadPoolExecutor executor = new ThreadPoolExecutor(AUTH_BLOCKING_MAX_THREADS,
+ AUTH_BLOCKING_MAX_THREADS, 60L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
Review Comment:
This queue is unbounded, so the executor bounds concurrency but not resource consumption. If a KDC stalls, every additional admin request retains another task/future indefinitely; enough requests can exhaust heap and affect the whole JVM despite the intended per-admin isolation. Use finite admission capacity with a policy that never runs rejected blocking work on the caller thread (for example, fail the future via rejection or implement asynchronous backpressure).
--
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] [fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #26327:
URL: https://github.com/apache/pulsar/pull/26327#issuecomment-5295273043
Thanks for the timing note — and for going through the fix rather than re-raising against the old
head. Taking the residual, because you are right that it is the one thing here that can regress
silently.
**`c5856643874`** adds `everyResourceTheAdminExposesIsLentThePool`, close to what you described:
package-private readers on both classes (`BaseResource.blockingAuthExecutorForTest()`,
`PulsarAdminImpl.blockingAuthExecutorForTest()`), and a walk that asserts every resource reports the
admin's pool.
One deviation from your sketch, for a reason worth stating. Rather than listing the accessors, the
test walks the ones `PulsarAdmin` itself declares — so it grows with the interface instead of with a
hand-maintained list, which would have the same forgetting problem one level up. It reflects over
public accessors only, never private state, so it stays inside the testing rule. Two guards against
the walk quietly covering nothing: `topicPolicies(boolean)` is checked explicitly, since it is the one
resource accessor taking an argument and the zero-arg filter cannot reach it; and the number of
resources actually reached is asserted, so a filter that excluded everything would fail rather than
pass vacuously.
Mutation-verified in the way that matters for this particular guard — I dropped `lendAuthExecutor`
from one construction site, reproducing precisely the "twenty-fourth resource" you describe:
```
java.lang.AssertionError: [admin resources left on the shared pool]
Expecting empty but was: ["scalableTopics"]
```
It names the accessor, so whoever trips it is told which line to fix rather than that something
somewhere is on the wrong pool.
On your read of the shared-pool interaction with #26328 — that is the part of your review that
survives its own staleness, and it is worth recording where it lands now. The proxy's broker-client
resolution still uses the shared pool, by design: it has no client to lend one. What changed is the
other end of the collision you described. With the admin off the shared pool, a burst of admin
requests against a stalled IdP can no longer occupy the eight shared workers that the proxy's backend
connection setup is queued behind. The remaining question — whether the proxy should own a pool
rather than borrow the shared one — is #26328's to answer, and I would rather it be asked there than
settled here.
`quickCheck` and `sanityCheck` pass; the class is 5 tests, green.
<sub>Prepared with the assistance of Claude Code (Opus 5).</sub>
--
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] [fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on code in PR #26327:
URL: https://github.com/apache/pulsar/pull/26327#discussion_r3785112322
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/BaseResource.java:
##########
@@ -137,6 +138,34 @@ protected CompletableFuture<Map<String, String>> computeAuthHeaders(URI uri) {
.thenApply(headers -> (headers == null || headers.isEmpty()) ? null : headers.asMap());
}
}
+ // The deprecated v4 hooks may block — an OAuth2 or Athenz shim's getAuthData() refreshes its
+ // credential with a synchronous HTTP exchange — and this method runs on whatever thread issued the
+ // admin call, which for a broker calling its own admin client is a request-handling thread.
+ // HttpClient.computeAuthHeaders off-loads the identical composition for the lookup path and names
+ // the self-deadlock it avoids; this was the last caller-thread credential resolution left, and the
+ // one place PIP-478's "every synchronous v4 plugin call is off-loaded" was still an overstatement.
+ //
+ // A null executor sends the work to the framework's shared blocking pool rather than running it
+ // inline. That is deliberate: BaseResource is constructed by 30 subclasses that would each have to
+ // thread an executor through, and the shared pool is exactly what PIP-478 provides for a component
+ // with none to lend. The admin's own bounded pool is still what a services-aware plugin gets, so
+ // the work that can actually stall — a KDC or IdP round trip inside the plugin — stays isolated
+ // per admin.
+ return V5AuthContexts.supplyBlocking(null, () -> v4AuthHeaders(uri)).thenCompose(headers -> headers);
Review Comment:
Accepted, fixed in `ef5aba0494b`.
You are right that this undercuts the PR's own purpose: the bounded pool exists to keep a stalled IdP
inside the admin that owns the plugin, and the v4 composition is the path that runs on *every* admin
request, so routing it to the process-wide pool left the common case exactly where it was.
My reason for `null` was cost — `BaseResource` is constructed 23 times, each subclass threading an
executor through — but that is a cost argument, not a correctness one, and it turns out to be
avoidable. `PulsarAdminImpl.lendAuthExecutor(...)` sets the executor on each resource immediately
after construction, so it is one wrapper per construction site in a single file and the 23 resource
classes are untouched. The pool is also created with the admin now rather than lazily, since it is on
the default path; an unused `ThreadPoolExecutor` starts no threads, so that costs one object.
Both of your line references (154 and 167) are covered — the continuation at 167 as well, see the
suppressed comment thread.
Pinned by `theV4CompositionRunsOnTheOwningAdminsOwnPool`, which builds a real `PulsarAdmin` so the
assertion covers the lending and not just `BaseResource`'s use of it. Reverting the lending fails it
with `"pulsar-auth-blocking-shared"`.
##########
pulsar-client-admin/src/main/java/org/apache/pulsar/client/admin/internal/PulsarAdminImpl.java:
##########
@@ -583,16 +594,46 @@ private void bindAuthenticationServices(ClientConfigurationData conf) {
() -> null, () -> null, () -> null, () -> authTlsFactory(conf), conf, clientInstanceId);
OpenTelemetry openTelemetry = conf.getOpenTelemetry() != null ? conf.getOpenTelemetry()
: OpenTelemetry.noop();
- // No scheduler: nothing on the admin path schedules periodic authentication work. The blocking
- // executor is left unbound deliberately too — a plugin that needs one falls back to the shared pool
- // rather than running on the caller thread, which is where the SASL-over-HTTP challenge rounds
- // off-load their GSSAPI work. Handing the admin's own request threads over instead would let a slow
- // KDC consume them.
+ // No scheduler: nothing on the admin path schedules periodic authentication work, and a plugin that
+ // does schedule some gets the framework's shared one — binding a scheduled pool per admin to sit idle
+ // would cost more than it buys.
+ //
+ // The blocking executor *is* the admin's own. This is where the SASL-over-HTTP challenge rounds put
+ // their GSSAPI work, so it must never be the admin's request threads — a slow KDC would consume them
+ // — but leaving it unbound is not right either: the plugin then shares one process-wide pool with
+ // every other client in the JVM, so a stalled identity provider reached by one admin throttles
+ // authentication for all of them. A small bounded pool per admin keeps that blast radius inside the
+ // admin that owns the plugin, and is shut down with it.
ClientAuthenticationServices services = new DefaultClientAuthenticationServices(
- authHttpClientFactory, null, null, Clock.systemDefaultZone(), openTelemetry, clientInstanceId);
+ authHttpClientFactory, null, blockingAuthExecutor(), Clock.systemDefaultZone(), openTelemetry,
+ clientInstanceId);
aware.bindClientAuthenticationServices(services);
}
+ /**
+ * The admin's bounded executor for potentially-blocking authentication work (PIP-478).
+ *
+ * <p>Queues rather than rejects: every caller is an authenticated request, so a saturated pool must slow
+ * requests down rather than fail them. Core threads time out, so an admin whose plugin never blocks pays
+ * for nothing. Created lazily under this admin's monitor and shut down in {@link #close()}.
+ *
+ * @return the blocking authentication executor
+ */
+ private synchronized Executor blockingAuthExecutor() {
+ if (blockingAuthExecutor == null) {
+ ThreadPoolExecutor executor = new ThreadPoolExecutor(AUTH_BLOCKING_MAX_THREADS,
+ AUTH_BLOCKING_MAX_THREADS, 60L, TimeUnit.SECONDS, new LinkedBlockingQueue<>(),
Review Comment:
Pushing back on this one, though not on its feasibility.
The queue cannot be the first thing to exhaust heap. A queued task is a supply node plus a captured
`URI` and future — hundreds of bytes — and each one corresponds to an admin request whose caller
already retains far more to have produced it: a synchronous admin call is a parked thread, ~1 MB of
stack. Reaching a queue depth that matters means the callers behind it have already committed orders
of magnitude more.
Rejection is also the wrong failure mode for this work: every caller is an authenticated request, so
a saturated pool should slow requests down rather than fail them. And `SharedBlockingExecutor` in
master makes the same choice for the same reason — bounding only the admin pool would leave two
admission policies for one class of work.
Your mechanism note is right, for the record: bounded admission would not need caller-runs, since
`V5AuthContexts.supplyBlocking` already turns a `RejectedExecutionException` into a failed future. So
this is a question of whether, not whether it can be done safely. If we want it, it should be one
configurable bound across both pools rather than an arbitrary capacity here. Tracked as a follow-up.
Javadoc updated to state the trade-off and its reasoning rather than just the choice.
--
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] [fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari merged PR #26327:
URL: https://github.com/apache/pulsar/pull/26327
--
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] [fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #26327:
URL: https://github.com/apache/pulsar/pull/26327#issuecomment-5295125217
Thanks — two of the three are right, and the first one is the more interesting: it says the PR
did not do what it claims.
Addressed in `ef5aba0494b`.
### 1. The v4 composition went to the shared pool — accepted, fixed
This is the finding that matters. The PR builds a bounded per-admin pool so that a stalled identity
provider reached through one admin's plugin cannot occupy the threads every other client in the JVM
shares — and then sends the composition that runs on **every** admin request to the process-wide
shared pool. The isolation covered the rare path (a services-aware plugin) and missed the common one.
My stated reason was that `BaseResource` is constructed 23 times and each subclass would have to
thread an executor through. That is a **cost** argument, not a correctness one, and the cost turns
out to be avoidable: `PulsarAdminImpl` now lends the executor to each resource immediately after
constructing it, so the change is one wrapper per construction site in one file and the 23 resource
classes are untouched.
Pinned by `theV4CompositionRunsOnTheOwningAdminsOwnPool`, which builds a real `PulsarAdmin` rather
than a bare `BaseResource` — so what it pins is that the admin actually lends its pool, not merely
that `BaseResource` would use one if given it. Reverting the lending fails it with
`"pulsar-auth-blocking-shared"` where `"pulsar-admin-auth-blocking"` is expected.
### 2. `newRequestHeader` on the completing thread — accepted, fixed, and fixed in both places
The suppressed comment on `v4AuthHeaders` was correct. `newRequestHeader` is the second synchronous
v4 hook, and a plugin of the multi-round challenge shape completes the stage from its own HTTP
callback thread — so off-loading the credential resolution alone covered half the composition. It is
now `thenApplyAsync(..., blockingExecutor)`.
Two notes on scope:
- In-tree this is third-party-plugin-only. `AuthenticationSasl` exposes `httpAuthenticationDriver()`
now, so the built-in multi-round path goes through the v5 driver above and never reaches the v4
hooks. Narrow — but third-party v4 plugins are exactly the population those hooks exist for.
- **`HttpClient` has the identical gap**, and it is already merged (#26282). The admin composition is
a mirror of it. I fixed it in the same commit: fixing one mirror and leaving the other is worse
than fixing neither. It is a one-line change in a file this PR did not otherwise touch — happy to
split it out if you would rather it travelled separately.
### 3. Unbounded queue — pushing back
> This queue is unbounded, so the executor bounds concurrency but not resource consumption.
True of the mechanism, but I do not think it is the binding constraint, and I would rather not
diverge from the shared pool on the strength of it:
- **The queue cannot be the first thing to exhaust heap.** A queued task is a supply node plus a
captured `URI` and future — hundreds of bytes. Every one of them corresponds to an admin request
whose caller *already* retains far more in order to have produced it: a synchronous admin call is a
parked thread, ~1 MB of stack. Anything that queues enough auth tasks to matter has already
committed one to three orders of magnitude more heap and stack to the requests behind them.
- **Rejection is the wrong failure mode here.** Every caller is an authenticated request. A saturated
pool should make requests slower, not fail them — a burst against a slow IdP would turn into failed
admin operations rather than delayed ones.
- **`SharedBlockingExecutor` already makes this choice**, with the same reasoning, in master. Bounding
only the admin pool would leave two different admission policies for one class of work.
To be clear about what I am *not* claiming: bounded admission is perfectly feasible without
caller-runs — `V5AuthContexts.supplyBlocking` already converts a `RejectedExecutionException` into a
failed future, so a rejected task never runs on the caller thread. My objection is to doing it here,
divergently, with an arbitrary capacity. If we want admission control on blocking authentication
work it should be one configurable bound across both pools. Tracked as a follow-up.
### One thing worth flagging about the tests
The mutation check caught a race in my own test before the review would have. The first version of
`theNewRequestHeaderHookIsOffLoadedToo` had the plugin complete the stage from a thread it started
itself — but when that completion wins the race, `thenApply` runs the continuation on the
*registering* thread, which is the blocking executor, and the assertion passes whether or not the fix
exists. Reverting the fix produced a **green** test, which is how I found it. The plugin now hands
the stage back to the test, which completes it after the continuation is registered. Both new tests
fail alone when their own fix is reverted.
<sub>Prepared with the assistance of Claude Code (Opus 5).</sub>
--
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] [fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor [pulsar]
Posted by "david-streamlio (via GitHub)" <gi...@apache.org>.
david-streamlio commented on PR #26327:
URL: https://github.com/apache/pulsar/pull/26327#issuecomment-5295180569
Timing note first: `ef5aba0494` landed about a minute before my review posted, so **my review reads against `c99d08f267` and all three of its findings are already fixed.** Nobody should chase them. Leaving this note so the thread is not misleading.
I went through the fix rather than assuming it, and it is complete — and in two places it goes further than what I raised:
- **All 23 construction sites are lent**, `scalableTopics` included, and `lendAuthExecutor` is `<T extends BaseResource>` so it cannot be applied to something that would not use it.
- **`HttpClient` got the same `thenApplyAsync` treatment.** I only flagged the split continuation in `BaseResource`; the identical gap existed in `HttpClient.computeAuthHeaders` — the method the PR body cites as the exemplar the admin was copying. Fixing the model as well as the copy is the right call and I had missed it.
- **The executor is now shut down on a failed build** too, in the `if (!constructed)` block, which nothing in the review asked for.
- The tests pin the right things: `theV4CompositionRunsOnTheOwningAdminsOwnPool` casts `admin.clusters()` off a real `PulsarAdmin`, so it covers the *lending* and not merely `BaseResource`'s use of what it is handed, and `theNewRequestHeaderHookIsOffLoadedToo` completes the stage from a separate `stage-completer` thread and asserts the hook still lands on `lent-auth-pool` — which is exactly the property that would otherwise have been invisible.
I also checked the one thing that looked risky on a first read: `HttpClient` passes its raw field where `BaseResource` passes `blockingExecutorOrShared(...)`, and its comment contemplates "no executor supplied", which would be an NPE if the field could be null. It cannot — the constructor normalises it to `Runnable::run`, so the comment is accurate as written. No issue.
On the queue: I had reached your conclusion independently before seeing your reply — my draft says to deprioritise it, on the same ground, that the queued task is trivial next to what the caller already retains to have produced it. Agreed that one configurable bound across both pools is the right shape if it is ever wanted, rather than an arbitrary capacity on the admin pool alone.
**One residual, forward-looking rather than a defect.** `lendAuthExecutor` is opt-in per construction line. A resource added to `PulsarAdminImpl` later without the wrapper compiles cleanly and silently falls back to the shared pool, reintroducing exactly what this commit fixed — and it would be invisible, since the fallback works. The new test exercises `clusters()`, so it would not catch a twenty-fourth resource that forgot it.
The cheap guard is a package-private getter next to the `setBlockingAuthExecutor` that already lives on `BaseResource`, and a test that walks the admin's public resource accessors asserting each reports the admin's pool. That keeps it inside Pulsar's "no reflection into private state" rule and makes forgetting the wrapper a test failure rather than a silent regression. Worth it only if you expect this list to keep growing; your call.
--
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] [fix][admin] PIP-478: off-load the admin's v4 credential and lend it a bounded auth executor [pulsar]
Posted by "lhotari (via GitHub)" <gi...@apache.org>.
lhotari commented on PR #26327:
URL: https://github.com/apache/pulsar/pull/26327#issuecomment-5357762624
**Restacked, plus one new commit.**
The branch was force-pushed after #26326 picked up the fixes from a whole-series final-state review, so the diff view has been reset — the code in this PR is unchanged by that restack.
The one new commit here is a `pip-478.md` correction that belongs to this PR specifically: the Error-model section still named `BaseResource` as "the exception" that "composes v4 headers on the calling thread" and described it as "tracked as a follow-up". This PR *is* that follow-up, so the document understated what shipped and left the surrounding normative sentence — "every synchronous v4 plugin call is off-loaded" — reading as false when it is now true.
Verified as part of the whole stack on [lhotari/pulsar#258](https://github.com/lhotari/pulsar/pull/258), 42/44 green (the two non-successes are that fork PR's non-semantic title and a skipped CodeQL run). apache/pulsar runs only the semantic-title check here until #26326 merges.
--
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