joseluisll commented on code in PR #8577:
URL: https://github.com/apache/hbase/pull/8577#discussion_r4237983567
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java:
##########
@@ -237,6 +237,7 @@ public class TestHRegion {
public static Configuration CONF;
private String dir;
private final int MAX_VERSIONS = 2;
+ private static final long THREAD_JOIN_TIMEOUT_NS =
TimeUnit.SECONDS.toNanos(5);
Review Comment:
Question: is 5s enough on a loaded CI host? On the success path the workers
are already done, so this limit mainly covers the flusher's last in-progress
`flush(true)` (the timed join + `assertFalse(flushThread.isAlive())` in the
increment/append tests) and how fast `FlushThread`/`PutThread` exit. A larger
value (e.g. 30s) costs nothing when threads stop promptly, and makes it less
likely that this introduces a new "did not stop" flake.
---
_Generated by [Claude Code](https://claude.ai/code)_
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegion.java:
##########
@@ -1346,18 +1382,14 @@ public void testGetWhileRegionClose() throws
IOException {
threads[i].start();
}
} finally {
+ done.set(true);
+ interruptAndJoinThreads(threads);
Review Comment:
Once `closing` is set, every `region.get()` fails right away in
`startRegionOperation` with `NotServingRegionException` ("is closing"), so each
getter thread ends on its own. Before this change, `done` stayed false until
after the close, so threads 5–9 (started after `closing.set(true)`) were sure
to make at least one get with the closing flag set, which is what this test
checks. With `done.set(true)` and the interrupt right after they start, those
threads may exit without ever calling `get`.
Could the `finally` first do a timed join *without* setting `done` (the
threads exit by themselves on NSRE), and only then `done.set(true);
interruptAndJoinThreads(threads);` as a fallback, before closing the region?
Optionally, also assert that each thread ended with a
`NotServingRegionException`, which proves it actually made a get with `closing`
set.
Separately, `GetTillDoneOrException` reads the mutable, non-volatile test
field `region`. Passing the `HRegion` into its constructor would rule out a
getter seeing `this.region == null` after cleanup. That race is narrow, but it
would show up as a false "Found a NPE".
---
_Generated by [Claude Code](https://claude.ai/code)_
--
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]
