PDGGK commented on PR #39711: URL: https://github.com/apache/beam/pull/39711#issuecomment-5248556159
@waterWang heads-up on an overlap, and an offer. I filed #39710 and then opened #39712 for it without re-checking — yours went up ten minutes before mine and I simply missed it. That's on me. Yours is first on the clock and the fix reads correct to me: same three sites, same "run every step, keep the first failure, suppress the rest" shape, and you got the `HBaseSharedConnection.close()` ordering right in the third one, which is the case that actually matters. The one thing yours doesn't have is tests, and CI is currently showing 2 failures. Mine carries `HBaseIOCloseTest` — 5 cases: - the two failure-collection helpers - one per teardown, each asserting the *later* resource is still released - the row-mutation one asserts `HBaseSharedConnection.getConnectionCount()` actually returns to zero, rather than verifying a mock, since the stranded refcount is the real consequence Reverting each teardown one at a time fails exactly and only its own test, so they're pinned per-site rather than as a block. **You're welcome to take that file wholesale** — it's at [`HBaseIOCloseTest.java`](https://github.com/apache/beam/pull/39712/files#diff-a0dbc7bd4f31d2cbb0e2a52d10dab1c1f4d3f1a8ac0ed37e4b1a29f0e0e9ba4e) in #39712; copy it in and I'll close mine. It needs `testImplementation library.java.mockito_core` added to `sdks/java/io/hbase/build.gradle` and three nested classes dropped from `private` to package-private so the test can construct them. If you'd rather not, that's fine too — I've said on my side that a committer should just pick one. No ownership claim here, I'd just like the fix to land with coverage. -- 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]
