merlimat commented on PR #26008: URL: https://github.com/apache/pulsar/pull/26008#issuecomment-4693057233
Thanks for the thorough review @lhotari — all findings addressed: 1. **Cross-layer validation gap (bug)** — fixed in 2 parts as suggested ([c09f665](https://github.com/merlimat/pulsar/commit/c09f6653b7c)): - The topic-level set now resolves against the **current namespace override** (cached read) and rejects the combination with 412. Covered by a new test that sets a conflicting namespace override, asserts the topic-level 412, and asserts the same topic override is accepted once the conflicting layer is removed. - The controller is resilient to a stored combination that is (or has become) invalid: `resolveAutoScaleConfig` catches the invariant violation, warn-logs the reason on each evaluation, and treats auto split/merge as **disabled** for the topic instead of failing the evaluation chain. Also covered by a new controller test. 2. **Overstated validation claim** — the misleading code comment is gone (replaced by one documenting the best-effort nature of the set-time check), and the PR description has been updated accordingly. 3. **CLI bindings** — intentional omission; now stated in the PR description as a follow-up PR (`CmdScalableTopics` / `CmdNamespaces`). 4. **GET 204 vs documented 200-empty** — the OpenAPI annotations on both GET endpoints now document the 204 no-override response ([2152340](https://github.com/merlimat/pulsar/commit/215234013c7)). 5. **Test hygiene** — this one is a non-issue in practice: `SharedPulsarBaseTest` creates a fresh namespace per test method and force-deletes it in `@AfterMethod`, so a mid-test failure cannot leak the `enabled=false` override into other tests — the cleanup is structural rather than per-test. (The new cross-layer test uses try/finally anyway, since it manipulates the namespace override mid-test.) -- 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]
