joseluisll commented on code in PR #8745:
URL: https://github.com/apache/hbase/pull/8745#discussion_r4238106911
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/SyncReplicationTestBaseNoBeforeAll.java:
##########
@@ -136,28 +136,39 @@ protected static void startClusters() throws Exception {
}
private static void shutdown(HBaseTestingUtil util) throws Exception {
- if (util.getHBaseCluster() == null) {
- return;
- }
- Admin admin = util.getAdmin();
- if (!admin.listReplicationPeers(Pattern.compile(PEER_ID)).isEmpty()) {
- if (
- admin.getReplicationPeerSyncReplicationState(PEER_ID)
- != SyncReplicationState.DOWNGRADE_ACTIVE
- ) {
- admin.transitReplicationPeerSyncReplicationState(PEER_ID,
- SyncReplicationState.DOWNGRADE_ACTIVE);
+ try {
+ if (util.getHBaseCluster() == null) {
+ return;
}
- admin.removeReplicationPeer(PEER_ID);
+ Admin admin = util.getAdmin();
+ if (!admin.listReplicationPeers(Pattern.compile(PEER_ID)).isEmpty()) {
+ if (
+ admin.getReplicationPeerSyncReplicationState(PEER_ID)
+ != SyncReplicationState.DOWNGRADE_ACTIVE
+ ) {
+ admin.transitReplicationPeerSyncReplicationState(PEER_ID,
+ SyncReplicationState.DOWNGRADE_ACTIVE);
+ }
+ admin.removeReplicationPeer(PEER_ID);
+ }
+ } finally {
+ // Always, even if the HBase part never came up: a failed
startMiniCluster leaves DFS running
Review Comment:
The case seen on #8743 was the early return: the retry failed in
`startMiniCluster()` with `getHBaseCluster() == null`, so DFS was left running
and the next rerun failed with "A mini-cluster is already running".
But `startMiniHBaseCluster()` assigns `hbaseCluster` right after
constructing it, before waiting for the `hbase:meta` scan. If startup fails
after that point, or a test fails in the middle of a peer state transition
(which is what the KillMaster tests exercise), the admin calls here run against
a broken cluster and throw, and `shutdownMiniCluster()` is skipped again.
Teardown only matters for reruns after a failure, so I'd like it to be safe in
that state.
I removed the `return` inside the `try` so it reads more simply (6684ac08c2).
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestAsyncWALReplay.java:
##########
@@ -66,8 +66,13 @@ public static void setUpBeforeClass() throws Exception {
@AfterAll
public static void tearDownAfterClass() throws Exception {
- if (GROUP != null) {
- GROUP.shutdownGracefully();
+ try {
+ if (GROUP != null) {
+ GROUP.shutdownGracefully();
+ }
+ } finally {
+ // this method hides the base one, so call it to shut down the mini
cluster
Review Comment:
Done, renamed to `tearDownAfterAll()` like `TestBulkLoadReplication`, so
JUnit runs both (subclass first). (6684ac08c2)
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/replication/TestClaimReplicationQueue.java:
##########
@@ -109,8 +109,13 @@ public static void setUpBeforeClass() throws Exception {
@AfterAll
public static void tearDownAfterClass() throws Exception {
- Closeables.close(table3, true);
- Closeables.close(table4, true);
+ try {
+ Closeables.close(table3, true);
+ Closeables.close(table4, true);
+ } finally {
+ // this method hides the base one, so call it to shut down the mini
clusters
+ TestReplicationBaseNoBeforeAll.tearDownAfterClass();
Review Comment:
Done, renamed to `tearDownAfterAll()`. `TestNamespaceReplication` had the
same problem (it hid the base teardown and never shut the clusters down), so I
renamed its method too. (6684ac08c2)
--
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]
