Release HBaseIO resources even when an earlier close() throws - #39712
Release HBaseIO resources even when an earlier close() throws#39712PDGGK wants to merge 1 commit into
Conversation
HBaseIO released its resources as consecutive, unguarded statements in three teardowns, so a throwing earlier close() skipped everything after it: HBaseReader.close() scanner.close() -> connection.close() HBaseWriterFn.tearDown() mutator.close() -> connection.close() WriteRowMutationsFn.tearDown() table.close() -> HBaseSharedConnection.close() The writer case is not a corner case: BufferedMutator.close() is documented to perform a flush, so any bundle whose final buffered write fails already leaked the Connection it created in @setup. The row-mutation case is worse than an ordinary leak. The skipped call is a reference-count decrement -- HBaseSharedConnection keeps a static pool, increments on getOrCreate and only closes the underlying Connection 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 unreleasable connection. 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, 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. HBaseReader, HBaseWriterFn and WriteRowMutationsFn drop `private` so the new test can construct them; they stay nested and are not part of any public API. Reverting each teardown individually fails exactly and only its own test. Fixes apache#39710
|
assign set of reviewers |
|
Assigning reviewers: R: @Abacn for label java. Note: If you would like to opt out of this review, comment Available commands:
The PR bot will only process comments in the main thread (not review comments). |
|
Flagging a collision I should have caught before opening this: #39711 by @waterWang covers the same three sites and was opened first. Timestamps, so nobody has to reconstruct them:
I checked for existing PRs on The two fixes are equivalent in approach: run every teardown step, keep the first failure, attach the rest as suppressed. The difference is that this one also adds I have no interest in a race here. If a committer prefers #39711, say so and I'll close this immediately; I've offered the tests over there so they can be folded in either way. Happy to go whichever direction gets the fix in with coverage. |
1 participant
Fixes #39710
HBaseIOreleased its resources as consecutive, unguarded statements in three teardowns, so a throwing earlierclose()skipped everything after it.HBaseReader.close()scanner.close()→connection.close()ConnectionHBaseWriterFn.tearDown()mutator.close()→connection.close()ConnectionWriteRowMutationsFn.tearDown()table.close()→HBaseSharedConnection.close()The writer case is not a corner case.
BufferedMutator.close()is documented as "Performs aflush()and releases any resources held.@throws IOExceptionif a remote or network exception occurs." So any bundle whose final buffered write fails already leaked theConnectioncreated in@Setup.The row-mutation case is worse than an ordinary leak. The skipped call is a reference-count decrement, not a close:
HBaseSharedConnectionkeeps astatic HashMappool,getOrCreateincrements the count, andclose(Configuration)closes the underlyingConnectiononly once the count reaches zero. Missing the decrement strands that entry — and its ZooKeeper session — for the lifetime of the JVM, and every latergetOrCreatehands 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/finallywould 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,HBaseWriterFnandWriteRowMutationsFndropprivateso 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:
HBaseReader.close()readerClosesTheConnectionWhenTheScannerFailsToCloseHBaseWriterFn.tearDown()writerClosesTheConnectionWhenTheFinalFlushFailsWriteRowMutationsFn.tearDown()rowMutationWriterReleasesTheSharedConnectionWhenTheTableFailsToClose:sdks:java:io:hbase:spotlessCheck,checkstyleMainandcheckstyleTestare clean.One thing I could not verify locally. In the full module run,
HBaseIOTestandHbaseIOWriteRowMutationsTestfail in@BeforeClassatMiniHBaseCluster.initwithjava.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 tomasterand 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.