lhotari commented on PR #25626: URL: https://github.com/apache/pulsar/pull/25626#issuecomment-4371835463
## Local Claude Code review _These comments come from a local [Claude Code](https://www.anthropic.com/claude-code) review run by @lhotari, not from a maintainer review. Treat them as suggestions to investigate, not blocking decisions._ ### Summary The optimization is correct and the algorithmic improvement (O(N²) → O(N) for full-namespace operations) is real. The cache invalidation story upstream is sound — `NamespaceBundleFactory.bundlesCache` is invalidated on bundle changes (`NamespaceBundleFactory.java:190`, `:199`, plus the `handleMetadataStoreNotification` listener at `:180`), so the cached validation is consistent with the prior `policies.bundles`-fed path in steady state. A few items worth a closer look before merge: ### 1. Dead `getNamespacePoliciesAsync` fetches — `NamespacesBase.java` In `internalGetTopicHashPositionsAsync` (around line 1601): ```java .thenCompose(__ -> getNamespacePoliciesAsync(namespaceName)) .thenCompose(policies -> { return validateNamespaceBundleOwnershipAsync(namespaceName, bundleRange, false, true) .thenCompose(...) }); ``` And in `internalUnsubscribeNamespaceBundleAsync` (around line 2077): ```java .thenCompose(__ -> getNamespacePoliciesAsync(namespaceName)) .thenCompose(policies -> validateNamespaceBundleOwnershipAsync(namespaceName, bundleRange, authoritative, false)) ``` In both places the `policies` lambda parameter is no longer referenced anywhere in the body, but the `getNamespacePoliciesAsync` call still incurs a metadata round-trip per request. This partially defeats the PR's optimization goal for these two endpoints. Consider removing the step (or replacing with `.thenCompose(__ -> ...)`). ### 2. Indentation glitch in the new `isBundleOwnedByAnyBroker` lambda — `PulsarWebResource.java:645-651` ```java if (ExtensibleLoadManagerImpl.isLoadManagerExtensionEnabled(pulsar)) { return nsService.checkOwnershipPresentAsync(nsBundle); // 23 spaces (off by 1) } LookupOptions options = LookupOptions.builder() .authoritative(false) // 24 spaces; rest of file .requestHttps(isRequestHttps()) // uses 8-space continuation ``` The `return` line is off by one space, and the chained-builder lines use a 4-space continuation indent where the rest of the file uses 8. Worth re-running `./gradlew spotlessCheck checkstyleMain` (you already had to fix Spotless once in `374ac3b1dc1`). ### 3. Subtle 404 → 412 shift on missing namespace In the call sites that *removed* `getNamespacePoliciesAsync` entirely (`internalUnloadNamespaceBundleAsync`, `internalSplitNamespaceBundleAsync`, `internalClearNamespaceBundleBacklogAsync`, `internalClearNamespaceBundleBacklogForSubscriptionAsync`), the prior `getNamespacePoliciesAsync` call also implicitly enforced "namespace exists in global config" (returns 404 on missing). The new path goes through `NamespaceBundleFactory.getBundlesAsync` → `loadBundles` → `copyToLocalPolicies`, which on a missing namespace falls back to `getBundles(namespace, Optional.empty())` — i.e. a *default* bundle layout. Then `validateBundle(nsBundle)` either succeeds (if the user-supplied range happens to match the default) or fails with `IllegalArgumentException` → `412 PRECONDITION_FAILED`. Net effect: clients calling these endpoints on a non-existent namespace will see `412` (or worse, accidental success on the default bundle range) instead of the expected `404`. Worth either restoring an explicit existence check up front, or documenting the new error semantics. Also a good candidate for a test (see #5). ### 4. `protected` API break is risky to backport The PR removes/renames protected hooks on `PulsarWebResource`: - `validateNamespaceBundleRange(NamespaceName, BundlesData, String)` removed - `validateNamespaceBundleOwnership(NamespaceName, BundlesData, String, boolean, boolean)` removed - `validateNamespaceBundleOwnershipAsync(...)` and `isBundleOwnedByAnyBroker(...)` lose the `BundlesData` parameter These are `protected`, but custom forks subclassing `PulsarWebResource` for auth/admin REST extensions are common in the wild. Dropping these hooks in `4.0.11` / `4.1.4` / `4.2.2` patch releases will break downstream builds silently after a minor upgrade. For the patch-release backports specifically, consider keeping deprecated bridge methods that delegate to the new ones — or restrict this change to master/next-minor. ### 5. Zero new tests The PR touches eight admin endpoints plus the protected helpers and is targeted for three patch branches, but adds no tests. At minimum: - An integration test asserting bundle validation still works correctly after a split via the cached path. - A test pinning down the error returned for a non-existent namespace (locks in whichever status code is intended — see #3). ### 6. Document the cache-correctness invariant — `PulsarWebResource.java:598` A one-line comment near the new `getBundlesAsync` call referencing the cache invalidation invariant (`NamespaceBundleFactory.bundlesCache` is invalidated on local-policy notifications) would future-proof the code: anyone who later breaks the cache invalidation would otherwise silently start validating against stale boundaries. --- **Already addressed in newer commits — kudos:** - The dead `catch (RestException e)` block (intermediate diff state) is gone in `294cfb32749`. - The inconsistent `thenComposeAsync` in `ResourceQuotasBase` is now plain `thenCompose`. - Unused `BundlesData` import already cleaned up in `374ac3b1dc1`. -- 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]
