[PR] [cleanup][misc] Fix all javac warnings in main sources [pulsar]

["aahmed-se (via GitHub)" <gi...@apache.org>]

aahmed-se opened a new pull request, #26257:
URL: https://github.com/apache/pulsar/pull/26257

   ### Motivation
   
   `./gradlew compileJava --warning-mode all` emits **77 javac warnings across ten modules** on a clean
   `master`. Almost all of them are deprecations left behind by the Netty 4.2, Jetty 12.1, Guava 33 and
   commons-lang3 3.20 upgrades:
   
   | Module | Warnings |
   |---|---|
   | `pulsar-client-admin-original` | 30 |
   | `pulsar-client-v5` | 17 |
   | `pulsar-broker` | 10 |
   | `pulsar-testclient` | 5 |
   | `pulsar-client-original` | 4 |
   | `pulsar-proxy` | 4 |
   | `pulsar-client-api` | 3 |
   | `pulsar-websocket` | 2 |
   | `pulsar-common` | 1 |
   | `pulsar-package-core` | 1 |
   
   Two reasons to clean this up now rather than later:
   
   1. **One warning is a latent break, not just noise.** Jetty deprecated
      `Session.Listener.onWebSocketClose(int, String)` *for removal*
      (`@Deprecated(since = "12.1.0", forRemoval = true)`). `AbstractWebSocketHandler` still overrides it,
      so the WebSocket proxy stops receiving close callbacks the moment Jetty drops the method.
   2. **The volume hides new warnings.** 52 of the 77 are the same repeated
      `unknown enum constant RequiredMode.REQUIRED` message, which drowns out anything genuinely new
      appearing in a PR's build log.
   
   ### Modifications
   
   **1. Deprecated APIs moved to their documented successors.** Each of these was checked against the
   dependency's own sources rather than assumed:
   
   - `CacheBuilder.expireAfterAccess/Write(long, TimeUnit)` → the `Duration` overloads, in the 7 caches in
     `NamespaceName`, `PackageName`, `PulsarClientImpl`, `AbstractMultiVersionReader`,
     `MultiVersionSchemaInfoProvider`, `ConsumerHandler` and `SimpleLoadManagerImpl`.
   - `PlatformDependent.threadLocalRandom()` → `ThreadLocalRandom.current()` in
     `PulsarServiceNameResolver`. Netty's deprecated method body is literally
     `return ThreadLocalRandom.current();`.
   - `ChannelOption.RCVBUF_ALLOCATOR` → `RECVBUF_ALLOCATOR` in `BrokerService` and `ProxyService`
     (4 call sites). A pure rename — Netty declares the deprecated field as `= RECVBUF_ALLOCATOR`, the
     same object.
   - `MutableObject.getValue()` → `get()` in `SystemTopicBasedTopicPoliciesService`.
   - The deprecated 5-arg `PersistentSubscription` constructor → the 4-arg one, in `PersistentTopic`.
     The 5-arg version ignores `subscriptionProperties` entirely (the cursor already carries them), so
     this is behaviour-identical. `createPersistentSubscription` keeps its signature because
     `BrokerTestInterceptor` overrides it; its javadoc now records that the parameter is unused.
   - Jetty's `onWebSocketClose(int, String)` → `onWebSocketClose(int, String, Callback)` in
     `AbstractWebSocketHandler`, completing the callback. The old override had to be *removed* rather
     than kept alongside the new one: `JettyWebSocketFrameHandlerFactory` throws
     `InvalidWebSocketException("Cannot use two versions of onWebSocketClose")` for an endpoint that
     declares both. Its `isOverridden` check keys on `getDeclaringClass() != Session.Listener.class`, so
     the inherited default is correctly ignored once ours is gone.
   - Removed the `EpollChannelOption.EPOLL_MODE` call in `DirectProxyHandler`. Netty 4.2 documents both
     the option and the `EpollMode` enum as no-ops ("Netty always uses level-triggered mode and so this
     method is just a no-op"), and level-triggered is exactly what zero-copy splice requires — so the
     guarded block was dead code. The `proxyZeroCopyModeEnabled` splice path itself is untouched.
   
   **2. One unchecked-generics fix.** `ScalableTopicController.prunable` built a
   `CompletableFuture<Boolean>[]` from a stream, which is an unchecked array creation. It now collects
   into a `List` and keeps the same short-circuiting drain check.
   
   **3. Narrow suppressions where no successor API exists** — placed at the declaration, each with a
   comment giving the reason:
   
   - `intercept(...)` in `ProducerBuilder` / `ConsumerBuilder` / `ReaderBuilder`: `@SafeVarargs` is
     illegal on an abstract method, so the heap-pollution warning is suppressed on the interface
     declaration. The implementations (`ConsumerBuilderImpl`, `ReaderBuilderImpl`) already carry
     `@SafeVarargs` on their `final` overrides.
   - The v5 `AuthenticationAdapter` bridge must call v4's deprecated host-less `getAuthData()` in order to
     implement v5's host-less `authData()`; there is no non-deprecated v4 equivalent without a broker
     host.
   - The three `TopicPoliciesService.registerListener` overrides
     (`SystemTopicBased`, `MetadataStore`, `LegacyAware`) are now `@Deprecated` themselves, matching the
     interface method they implement.
   
   **4. `compileOnly(swagger-annotations)` added to three modules** — this is the 52-warning group.
   `pulsar-client`'s configuration-data classes (`ClientConfigurationData` and friends) carry
   runtime-retained `@Schema(requiredMode = REQUIRED)` annotations. `pulsar-client-admin`,
   `pulsar-client-v5` and `pulsar-testclient` compile against those class files without the annotation
   types on their compile classpath, so javac can't resolve the enum constant. Six other modules
   (`pulsar-broker`, `pulsar-common`, `pulsar-client`, `pulsar-proxy`, `pulsar-websocket`,
   `pulsar-client-tools`) already declare it exactly this way; this just extends the existing convention.
   
   No production code behaviour is intended to change anywhere in this PR.
   
   **Compatibility note:** `AbstractWebSocketHandler.onWebSocketClose` changes signature. That class is
   `public` in `pulsar-websocket`, so an out-of-tree subclass overriding the 2-arg form would need the
   same one-line migration — but Jetty forces this move regardless, and the method being overridden is
   Jetty's listener callback rather than a Pulsar API. I have not ticked *The public API* below for this
   reason; happy to reclassify if a reviewer sees it differently.
   
   ### Verifying this change
   
   - [x] Make sure that the change passes the CI checks.
   
   *(Please pick either of the following options)*
   
   This change is already covered by existing tests, such as:
   
   - **`ProxyPublishConsumeTest`** (18 tests) — the end-to-end WebSocket path against a real Jetty
     server. This is the important one: it exercises endpoint registration (which is where a botched
     `onWebSocketClose` migration throws `InvalidWebSocketException`) and real session closes.
   - **The full `pulsar-websocket` suite** (33 tests), including `AbstractWebSocketHandlerTest`, whose
     direct `onWebSocketClose` call was updated to pass `Callback.NOOP`.
   - **`NamespaceNameTest`, `PackageNameTest`, `PulsarServiceNameResolverTest`** (29 tests) — the Guava
     `Duration` and `ThreadLocalRandom` changes.
   - **`PersistentSubscriptionTest`, `CreateSubscriptionTest`, `ScalableTopicControllerTest`,
     `SimpleLoadManagerImplTest`, `ProxyTest`, `ProxyDisableZeroCopyTest`** (90 tests) — the
     `PersistentSubscription` constructor, the `prunable` rewrite, the load-manager cache and the Netty
     channel-option renames.
   - **`SystemTopicBasedTopicPoliciesServiceTest`, `LegacyAwareTopicPoliciesServiceTest`,
     `TopicPoliciesServiceDisableTest`, `InmemoryTopicPoliciesServiceServiceTest`** (27 tests) — the
     `MutableObject.get()` change sits on the namespace-cleanup path these cover.
   
   197 tests total, 0 failures and 0 errors. No new tests were added: every change is either a
   like-for-like API swap or annotation-only, so there is no new behaviour to cover.
   
   Build verification:
   
   - `./gradlew compileJava --rerun-tasks --warning-mode all` → **BUILD SUCCESSFUL with zero warnings**
     (was 77).
   - `./gradlew sanityCheck` → passes (license headers, checkstyle, and main *and* test compilation for
     every module).
   
   Two things a reviewer should know:
   
   - **The `DirectProxyHandler` change is not covered by a local test run.** epoll is Linux-only, so
     `proxyZeroCopyModeEnabled` never engages on macOS and `ProxyDisableZeroCopyTest` only covers the
     disabled path. The removal rests on Netty 4.2's own javadoc marking the option a no-op. CI on Linux
     will exercise the zero-copy path.
   - **Test sources still emit warnings** — roughly 197 across 14 `compileTestJava` tasks, mostly
     unchecked conversions in Mockito stubs, plus the same swagger classpath issue in `pulsar-proxy`'s
     tests. Deliberately left out to keep this PR reviewable; happy to follow up separately.
   
   ### Does this pull request potentially affect one of the following parts:
   
   *If the box was checked, please highlight the changes*
   
   - [x] 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
   
   **Dependencies:** `compileOnly(libs.swagger.annotations)` is added to `pulsar-client-admin`,
   `pulsar-client-v5` and `pulsar-testclient`. Nothing is added or upgraded at the project level — the
   artifact and its version already come from the version catalog and are already used by six other
   modules. Because the scope is `compileOnly`, it reaches neither the runtime classpath nor the
   published artifacts: I regenerated all three POMs with `generatePomFileForMavenPublication` and
   confirmed `swagger` appears zero times in each. No distribution contents or LICENSE/NOTICE files
   change. Flagged only because the diff touches `build.gradle.kts` files.


-- 
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] [cleanup][misc] Fix all javac warnings in main sources [pulsar]

["lhotari (via GitHub)" <gi...@apache.org>]

lhotari commented on code in PR #26257:
URL: https://github.com/apache/pulsar/pull/26257#discussion_r3893489850


##########
pulsar-websocket/src/main/java/org/apache/pulsar/websocket/AbstractWebSocketHandler.java:
##########
@@ -262,7 +262,7 @@ public void onWebSocketError(Throwable cause) {
     }
 
     @Override
-    public void onWebSocketClose(int statusCode, String reason) {
+    public void onWebSocketClose(int statusCode, String reason, Callback callback) {

Review Comment:
   **[BUG] the onWebSocketClose signature change is an undeclared public-API break that fails at runtime, not at compile time**
   
   `pulsar-websocket` is a **published** Maven artifact — `build.gradle.kts:21` applies `pulsar.public-java-library-conventions`, whose own header reads "Convention plugin for public Pulsar Java libraries that are published to Maven repositories… Internal-only modules should use pulsar.java-conventions directly" — and `AbstractWebSocketHandler` is `public` (line 65). So this is a public-API change. The PR checklist leaves **"The public API"** unticked, ticking only "Dependencies", and the description frames the impact as "an out-of-tree subclass overriding the 2-arg form would need the same one-line migration".
   
   The real failure mode is worse than a migration: it compiles and fails at runtime.
   
   **Failure mode 1 — a subclass stops being registrable.** A downstream:
   
   ```java
   public class MyHandler extends AbstractWebSocketHandler {
       @Override public void onWebSocketClose(int statusCode, String reason) { … }
   }
   ```
   
   still compiles against the new Pulsar (exit 0, one `[removal]` warning), because `Session.Listener.onWebSocketClose(int, String)` is still present as a `default` — jetty-websocket-jetty-api 12.1.11, `Session.java:341`, `@Deprecated(since = "12.1.0", forRemoval = true)` — so the `@Override` stays valid. At endpoint registration `JettyWebSocketFrameHandlerFactory` (12.1.11:239-244) then finds *both* methods and rejects it:
   
   ```java
   Method deprecatedCloseMethod = findMethod(endpointClass, "onWebSocketClose", int.class, String.class);
   Method closeMethod = findMethod(endpointClass, "onWebSocketClose", int.class, String.class, Callback.class);
   if (closeMethod != null) {
       if (deprecatedCloseMethod != null)
           throw new InvalidWebSocketException("Cannot use two versions of onWebSocketClose");
   ```
   
   `findMethod` delegates to `ReflectUtils.findMethod` (`Class.getMethod`, so inherited methods are seen) and filters only through `isOverridden(m)` = `m.getDeclaringClass() != Session.Listener.class` (factory:317-320). The subclass's own 2-arg declaration counts; the 3-arg method inherited from `AbstractWebSocketHandler` counts too. Before this PR the pair could not occur, because Pulsar declared only the 2-arg form.
   
   *Scenario:* a downstream WebSocket-proxy extension subclassing `AbstractWebSocketHandler` upgrades Pulsar, rebuilds with no error, and every WebSocket upgrade then fails with `InvalidWebSocketException: Cannot use two versions of onWebSocketClose`.
   
   **Failure mode 2 — a direct 2-arg caller silently no-ops.** External code or a test harness calling `handler.onWebSocketClose(code, reason)` on an `AbstractWebSocketHandler` reference still compiles (again with a `[removal]` warning) but now resolves to Jetty's **empty default** instead of Pulsar's cleanup, so `pingFuture` is never cancelled and `close()` is never called. This PR's own `AbstractWebSocketHandlerTest:399` was exactly such a caller and had to be updated.
   
   **To be clear about what I am *not* asking for.** The migration is correct and forced: Jetty deprecated the 2-arg form *for removal*, and keeping both forms on `AbstractWebSocketHandler` is precisely what the factory above rejects — so there is no source-compatible way to have both. The choice is:
   
   1. **Migrate now** (as here) and declare it: tick "The public API", name the runtime symptom (`InvalidWebSocketException: Cannot use two versions of onWebSocketClose`, thrown at endpoint registration) so downstream maintainers can search for it, and consider a prefix other than `[cleanup][misc]` — the Jetty 12.1 upgrade itself went in as `[improve][misc]` (#25100).
   2. **Defer it**: keep the 2-arg override for now and suppress the `[removal]` warning, the way #25421 (`[improve][build] Suppress deprecation warnings across all modules`) handled this class of warning. That keeps this PR entirely behaviour-neutral and moves the break to a release where it can be called out on its own.
   
   Either is defensible; the current state — a real break, undeclared, inside a `[cleanup]` PR — is the one option I would not merge as-is.



-- 
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