merlimat opened a new pull request, #25694: URL: https://github.com/apache/pulsar/pull/25694
### Motivation `PortManager` (in `pulsar-common`) and the inner `BasePortManager` of `LocalBookkeeperEnsemble` allocated "free" ports via `ServerSocket(0)` and tracked them in a JVM-level lock set. The lock was JVM-only, so the OS-level port could still be grabbed by another process between allocation and bind, producing flaky `BindException`s in tests (most recently `ShadowTopicRealBkTest.setup`). The class has outlived its usefulness — modern BookKeeper identifies bookies by `bookieId`, not host:port, so kernel-assigned ports (port 0) work across the board. The "lock" never actually prevented OS-level collisions anyway. ### Modifications **Removed** - `pulsar-common/.../PortManager.java` and its test. - `pulsar-package-management/.../test/PortManager.java` (unused duplicate). - `BasePortManager` inner class in `LocalBookkeeperEnsemble`. - All `bkBasePort` constructor variants of `LocalBookkeeperEnsemble`. - The `Supplier<Integer> portManager` parameter from `LocalBookkeeperEnsemble` — every caller was passing `() -> 0`. Bookies now always bind to kernel-assigned ports internally. - `--bookkeeper-port` CLI option from `pulsar standalone` (and its setter, getter, builder method, plus the test calls). Multi-bookie standalone gets kernel-assigned ports. - `useDynamicBrokerPorts()` toggle in `MultiBrokerBaseTest` (now always dynamic). - `bkPort` field in `BKCluster.BKClusterConf`. **Refactored** - `BKCluster`: bookies bind to port 0; `bookieId` is derived from the data dir hash so multiple cluster instances on a shared metadata store don't collide on cookie validation, while restarts on the same dir keep cookies stable. The unused cookie-parsing port-recovery logic is dropped. - `MultiBrokerBaseTest`: `mainBrokerPort` and `additionalBrokerPorts` are populated post-start from `pulsar.getBrokerListenPort()` instead of pre-allocated. - `PulsarTestContext.handlePreallocatePorts`: inlined `ServerSocket(0)` allocation. Still needed by tests that build advertised-listener URLs before the broker starts (those URLs require a known port up front). - 17 test files updated: `PortManager.nextLockedFreePort()` calls replaced with `0` (let kernel pick) or inline `ServerSocket(0)` (only where pre-allocation is genuinely required, e.g. for advertised-listener URLs). - `PulsarStandaloneTest.testStandaloneWithRocksDB`: assertion switched from `getBookiePort()` (now returns kernel-assigned ports that change per restart) to `getBookieId()` (the persistent BK identity). ### Behavior change `--bookkeeper-port` is gone from `pulsar standalone`. Bookies get kernel-assigned ports — no impact for normal usage since clients only connect to the broker, not directly to bookies. ### Verifying this change Locally exercised the affected tests: - `LocalBookkeeperEnsembleTest` (2/2) - `ShadowTopicRealBkTest` (1/1) - `PulsarStandaloneTest` (5/5) - `EndToEndTest` for `BKCluster` (10 passed, 1 skipped) - `AdvertisedListenersTest`, `LoadManagerFailFastTest`, `AdvertisedListenersMultiBrokerLeaderElectionTest` Full `compileJava` and `compileTestJava` clean. ### Does this pull request potentially affect one of the following parts: - [ ] Dependencies (does it add or upgrade a dependency): no - [x] The public API: removes `--bookkeeper-port` CLI option from `pulsar standalone` - [ ] The schema: no - [ ] The default values of configurations: no - [ ] The threading model: no - [ ] The binary protocol: no - [ ] The REST endpoints: no - [x] The admin CLI options: `--bookkeeper-port` removed from `pulsar standalone` - [ ] The metrics: no - [ ] Anything that affects deployment: no -- 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]
