PDGGK opened a new pull request, #39712: URL: https://github.com/apache/beam/pull/39712
Fixes #39710 `HBaseIO` released its resources as consecutive, unguarded statements in three teardowns, so a throwing earlier `close()` skipped everything after it. | | | leaked | |---|---|---| | `HBaseReader.close()` | `scanner.close()` → `connection.close()` | `Connection` | | `HBaseWriterFn.tearDown()` | `mutator.close()` → `connection.close()` | `Connection` | | `WriteRowMutationsFn.tearDown()` | `table.close()` → `HBaseSharedConnection.close()` | shared connection, permanently | **The writer case is not a corner case.** `BufferedMutator.close()` is documented as *"Performs a `flush()` and releases any resources held. `@throws IOException` if a remote or network exception occurs."* So any bundle whose final buffered write fails already leaked the `Connection` created in `@Setup`. **The row-mutation case is worse than an ordinary leak.** The skipped call is a reference-count decrement, not a close: `HBaseSharedConnection` keeps a `static HashMap` pool, `getOrCreate` increments the count, and `close(Configuration)` closes the underlying `Connection` only once the count reaches zero. Missing the decrement strands that entry — and its ZooKeeper session — for the lifetime of the JVM, and every later `getOrCreate` hands back the same connection that can no longer ever be released. ### Approach Each teardown now runs every step and keeps the **first** failure, attaching later ones as suppressed. A plain nested `try/finally` would guarantee the calls happen but would silently swap which exception the caller sees — a teardown symptom would replace the write error that actually broke the job — so it is only half a fix. The collection and rethrow logic is shared by the three sites as two package-private helpers on `HBaseIO`. There is no existing beam-wide utility for this; other IOs (`iceberg`, `arrow-flight`, `google-cloud-platform`) each do it inline, so keeping it local to this connector matches what the codebase already does. `HBaseReader`, `HBaseWriterFn` and `WriteRowMutationsFn` drop `private` so the test can construct them. They stay nested and are not exposed in any public API; this seemed better than driving them through reflection from the test. ### Testing New `HBaseIOCloseTest`, 5 cases: two for the failure-collection helpers, one per fixed teardown. The row-mutation case asserts the **real reference count** returns to zero rather than a mock interaction, since that is the actual consequence being fixed. Reverting each teardown individually — one at a time, not all three at once — fails **exactly and only** its own test: | reverted | tests that fail | |---|---| | `HBaseReader.close()` | `readerClosesTheConnectionWhenTheScannerFailsToClose` | | `HBaseWriterFn.tearDown()` | `writerClosesTheConnectionWhenTheFinalFlushFails` | | `WriteRowMutationsFn.tearDown()` | `rowMutationWriterReleasesTheSharedConnectionWhenTheTableFailsToClose` | `:sdks:java:io:hbase:spotlessCheck`, `checkstyleMain` and `checkstyleTest` are clean. **One thing I could not verify locally.** In the full module run, `HBaseIOTest` and `HbaseIOWriteRowMutationsTest` fail in `@BeforeClass` at `MiniHBaseCluster.init` with `java.io.IOException: Shutting down` — the mini-cluster does not come up on my machine (Apple Silicon). I confirmed this is not caused by this change by restoring both files to `master` and removing the new test: the same two classes fail identically. The other 19 tests in the module pass, including all 5 new ones. CI should be the judge of the two mini-cluster classes. -- 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]
