DieterDePaepe commented on code in PR #8694:
URL: https://github.com/apache/hbase/pull/8694#discussion_r4166563305
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -248,6 +264,59 @@ private void performSnapshots(Admin admin) throws
IOException {
}
}
+ /**
+ * Scan all WAL files to find offline/decommissioned hosts and record their
max WAL timestamps in
+ * newTimestamps. This ensures subsequent incremental backups won't
reinclude WALs already covered
+ * by this full backup's snapshot.
+ */
+ private void adjustTimestampsForOfflineHosts(Map<String, Long>
previousLogRollsByHost,
Review Comment:
I think this change introduces a new possible data loss scenario, where a
region server comes online after the log roll and before the WAL listing (ie:
while the `performBackupSnapshots` is running the MR copy jobs, which can
significant for full backups).
This test demonstrates the issue:
```java
public class TestBackupOfflineRS extends TestBackupBase {
...
/** Full backup client that runs {@link #afterSnapshotHook} once the table
snapshots exist. */
public static class FullTableBackupClientWithHook extends
FullTableBackupClient {
@Override
protected void snapshotCopy(BackupInfo backupInfo) throws IOException {
afterSnapshotHook.run();
super.snapshotCopy(backupInfo);
}
}
/**
* Tests that edits written during a full backup, after its log roll and
table snapshot, to an RS
* that started after that log roll, are included in the next incremental
backup.
*/
@Test
public void testRSStartedDuringFullBackup() throws Exception {
SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
List<TableName> tables = Lists.newArrayList(table1);
afterSnapshotHook = () -> {
try {
HRegionServer newRS =
cluster.startRegionServerAndWait(10000).getRegionServer();
try (Admin admin = TEST_UTIL.getConnection().getAdmin()) {
RegionInfo region = admin.getRegions(table1).get(0);
admin.move(region.getEncodedNameAsBytes(), newRS.getServerName());
TEST_UTIL.waitUntilAllRegionsAssigned(table1);
}
try (Connection conn = ConnectionFactory.createConnection(conf1)) {
insertIntoTable(conn, table1, famName, 8, 50).close();
}
} catch (Exception e) {
throw new RuntimeException(e);
}
};
conf1.set(TableBackupClient.BACKUP_CLIENT_IMPL_CLASS,
FullTableBackupClientWithHook.class.getName());
String fullBackupId;
try {
fullBackupId = fullTableBackup(tables);
} finally {
conf1.unset(TableBackupClient.BACKUP_CLIENT_IMPL_CLASS);
}
assertTrue(checkSucceeded(fullBackupId), "Full backup should succeed");
String incrBackupId = incrementalTableBackup(tables);
assertTrue(checkSucceeded(incrBackupId), "Incremental backup should
succeed");
TableName restoredTable =
TableName.valueOf("table1_rs_started_during_full_backup");
try (Connection conn = ConnectionFactory.createConnection(conf1);
BackupAdminImpl backupAdmin = new BackupAdminImpl(conn)) {
backupAdmin.restore(BackupUtils.createRestoreRequest(BACKUP_ROOT_DIR,
incrBackupId, false,
new TableName[] { table1 }, new TableName[] { restoredTable },
true));
}
assertEquals(TEST_UTIL.countRows(table1),
TEST_UTIL.countRows(restoredTable),
"Restored table should contain all rows, including those written to
the new RS");
}
```
This test succeeded on the original version.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+
+/**
+ * Tests that WAL files from offline/inactive RegionServers are handled
correctly during backup.
+ * Specifically verifies that WALs from an offline RS are:
+ * <ol>
+ * <li>Backed up once in the first backup after the RS goes offline</li>
+ * <li>NOT re-backed up in subsequent backups</li>
+ * </ol>
+ */
+@Tag(LargeTests.TAG)
+public class TestBackupOfflineRS extends TestBackupBase {
Review Comment:
These tests fail on my computer, and I traced it down to my hostname
containing a capital. Apparently, the backup code is mixing lower and
original-cased hostnames.
- Lowercased: the server-name `host,port,startcode`, and WAL folder/file
names.
- Original case: `ServerName.getHostname()` and `getAddress().toString()`,
which end up in the roll results.
So on a host with uppercase letters in its name, roll-result keys and
WAL-path keys don't match.
The simplest option is to lowercase the hostname wherever the backup code
builds a host:port key: in logRollV2, in LogRollBackupSubprocedure, and in the
two BackupUtils parsers, so all of them agree with the WAL file names. Already
stored rslogts and trslm rows would need to be read case-insensitively, or
migrated once.
My workaround (which can be adjusted to trigger the issue too):
```
conf1.set("hbase.unsafe.regionserver.hostname", "localhost");
conf1.set("hbase.master.hostname", "localhost");
```
Fine for me to log this as a separate issue instead of fixing here. Let me
know if you'd prefer I log a new ticket for this.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
Review Comment:
These tests don't demonstrate the actual data loss scenario from the ticket,
just the changed bookkeeping.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+
+/**
+ * Tests that WAL files from offline/inactive RegionServers are handled
correctly during backup.
+ * Specifically verifies that WALs from an offline RS are:
+ * <ol>
+ * <li>Backed up once in the first backup after the RS goes offline</li>
+ * <li>NOT re-backed up in subsequent backups</li>
+ * </ol>
+ */
+@Tag(LargeTests.TAG)
+public class TestBackupOfflineRS extends TestBackupBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestBackupOfflineRS.class);
+
+ @BeforeAll
+ public static void setUp() throws Exception {
+ TEST_UTIL = new HBaseTestingUtil();
+ conf1 = TEST_UTIL.getConfiguration();
+ conf1.setInt("hbase.regionserver.info.port", -1);
+ autoRestoreOnFailure = true;
+ useSecondCluster = false;
+ setUpHelper();
+ TEST_UTIL.getMiniHBaseCluster().startRegionServer();
+ TEST_UTIL.waitTableAvailable(table1);
+ }
+
+ /**
+ * Tests that when a full backup is taken while an RS is offline (with WALs
in oldlogs), the
+ * offline host's timestamps are recorded so subsequent incremental backups
don't reinclude those
+ * WALs.
+ */
+ @Test
+ public void testBackupWithOfflineRS() throws Exception {
+ LOG.info("Starting testBackupWithOfflineRS");
+
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ LOG.info("Inserting data to generate WAL entries");
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 2, 100);
+ }
+
+ int rsToStop = 0;
+ HRegionServer rsBeforeStop = cluster.getRegionServer(rsToStop);
+ String offlineHost =
+ rsBeforeStop.getServerName().getHostname() + ":" +
rsBeforeStop.getServerName().getPort();
+ LOG.info("Stopping RS: {}", offlineHost);
+
+ cluster.stopRegionServer(rsToStop);
+ Thread.sleep(5000);
+
+ LOG.info("Taking full backup (with offline RS WALs in oldlogs)");
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue(checkSucceeded(fullBackupId), "Full backup should succeed");
+
+ try (BackupSystemTable sysTable = new
BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps =
sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ LOG.info("RS timestamps after full backup: {}", rsTimestamps);
+
+ Long tsAfterFullBackup = rsTimestamps.get(offlineHost);
+ assertNotNull(tsAfterFullBackup,
+ "Offline host should have timestamp recorded in trslm after full
backup");
+
+ LOG.info("Taking incremental backup (should NOT include offline RS
WALs)");
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue(checkSucceeded(incrBackupId), "Incremental backup should
succeed");
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ rsTimestamps = timestamps.get(table1);
+ assertFalse(rsTimestamps.containsKey(offlineHost),
+ "Offline host should not have a boundary after incremental");
+ }
+ }
+
+ /**
+ * Tests that WALs written to an RS after a full backup are correctly
included in the subsequent
+ * incremental backup, even if that RS has gone offline before the
incremental runs.
+ */
+ @Test
+ public void testRSGoesOfflineAfterFullBackupBeforeIncremental() throws
Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 3, 50);
+ }
+
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue(checkSucceeded(fullBackupId), "Full backup should succeed");
+
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 4, 50);
+ }
+
+ HRegionServer rsBeforeStop =
cluster.getLiveRegionServerThreads().get(0).getRegionServer();
+ rsBeforeStop.getWalRoller().requestRollAll();
+ rsBeforeStop.getWalRoller().waitUntilWalRollFinished();
+ String offlineHost =
+ rsBeforeStop.getServerName().getHostname() + ":" +
rsBeforeStop.getServerName().getPort();
+
+ cluster.stopRegionServer(rsBeforeStop.getServerName());
+ Thread.sleep(5000);
+
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue(checkSucceeded(incrBackupId), "Incremental backup should
succeed");
+
+ try (BackupSystemTable sysTable = new
BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps =
sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ assertNotNull(rsTimestamps.get(offlineHost),
+ "Offline RS should have a timestamp boundary after the incremental
backed up its WALs");
+
+ String incrBackupId2 = incrementalTableBackup(tables);
+ assertTrue(checkSucceeded(incrBackupId2), "Second incremental backup
should succeed");
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ rsTimestamps = timestamps.get(table1);
+ assertFalse(rsTimestamps.containsKey(offlineHost),
+ "Offline RS should not have a boundary after all its WALs have been
backed up");
+ }
+ }
+
+ /**
+ * Tests that a brand-new RS that comes online and goes offline before any
backup correctly has its
+ * WALs covered by the full backup.
+ */
+ @Test
+ public void testTransientRSBeforeFullBackup() throws Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ HRegionServer transientRS =
cluster.startRegionServerAndWait(10000).getRegionServer();
+ try (Admin admin = TEST_UTIL.getConnection().getAdmin()) {
+ List<RegionInfo> regions = admin.getRegions(table1);
+ if (!regions.isEmpty()) {
Review Comment:
(copied from the original PR):
I think this check cannot be empty - a default region gets created when a
table is created. Having this check here hints that it would be a valid option
to have no regions - but in that case there would be no guarantee of an active
region being present on transientRS.
I suggest to replace this with an assertFalse.
Also for line 244.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+
+/**
+ * Tests that WAL files from offline/inactive RegionServers are handled
correctly during backup.
+ * Specifically verifies that WALs from an offline RS are:
+ * <ol>
+ * <li>Backed up once in the first backup after the RS goes offline</li>
+ * <li>NOT re-backed up in subsequent backups</li>
+ * </ol>
+ */
+@Tag(LargeTests.TAG)
+public class TestBackupOfflineRS extends TestBackupBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestBackupOfflineRS.class);
+
+ @BeforeAll
+ public static void setUp() throws Exception {
+ TEST_UTIL = new HBaseTestingUtil();
+ conf1 = TEST_UTIL.getConfiguration();
+ conf1.setInt("hbase.regionserver.info.port", -1);
+ autoRestoreOnFailure = true;
+ useSecondCluster = false;
+ setUpHelper();
+ TEST_UTIL.getMiniHBaseCluster().startRegionServer();
+ TEST_UTIL.waitTableAvailable(table1);
+ }
+
+ /**
+ * Tests that when a full backup is taken while an RS is offline (with WALs
in oldlogs), the
+ * offline host's timestamps are recorded so subsequent incremental backups
don't reinclude those
+ * WALs.
+ */
+ @Test
+ public void testBackupWithOfflineRS() throws Exception {
+ LOG.info("Starting testBackupWithOfflineRS");
+
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ LOG.info("Inserting data to generate WAL entries");
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 2, 100);
+ }
+
+ int rsToStop = 0;
+ HRegionServer rsBeforeStop = cluster.getRegionServer(rsToStop);
Review Comment:
`cluster.getRegionServer(0)` indexes `getRegionServers()`, and that list
includes stopped servers. If
`testRSGoesOfflineAfterFullBackupBeforeIncremental` were to run first, this
might be stopping a server that is already stopped, so not the scenario you
intend to have.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+
+/**
+ * Tests that WAL files from offline/inactive RegionServers are handled
correctly during backup.
+ * Specifically verifies that WALs from an offline RS are:
+ * <ol>
+ * <li>Backed up once in the first backup after the RS goes offline</li>
+ * <li>NOT re-backed up in subsequent backups</li>
+ * </ol>
+ */
+@Tag(LargeTests.TAG)
+public class TestBackupOfflineRS extends TestBackupBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestBackupOfflineRS.class);
+
+ @BeforeAll
+ public static void setUp() throws Exception {
+ TEST_UTIL = new HBaseTestingUtil();
+ conf1 = TEST_UTIL.getConfiguration();
+ conf1.setInt("hbase.regionserver.info.port", -1);
+ autoRestoreOnFailure = true;
+ useSecondCluster = false;
+ setUpHelper();
+ TEST_UTIL.getMiniHBaseCluster().startRegionServer();
+ TEST_UTIL.waitTableAvailable(table1);
+ }
+
+ /**
+ * Tests that when a full backup is taken while an RS is offline (with WALs
in oldlogs), the
+ * offline host's timestamps are recorded so subsequent incremental backups
don't reinclude those
+ * WALs.
+ */
+ @Test
+ public void testBackupWithOfflineRS() throws Exception {
+ LOG.info("Starting testBackupWithOfflineRS");
+
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ LOG.info("Inserting data to generate WAL entries");
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 2, 100);
+ }
+
+ int rsToStop = 0;
+ HRegionServer rsBeforeStop = cluster.getRegionServer(rsToStop);
+ String offlineHost =
+ rsBeforeStop.getServerName().getHostname() + ":" +
rsBeforeStop.getServerName().getPort();
+ LOG.info("Stopping RS: {}", offlineHost);
+
+ cluster.stopRegionServer(rsToStop);
+ Thread.sleep(5000);
+
+ LOG.info("Taking full backup (with offline RS WALs in oldlogs)");
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue(checkSucceeded(fullBackupId), "Full backup should succeed");
+
+ try (BackupSystemTable sysTable = new
BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps =
sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ LOG.info("RS timestamps after full backup: {}", rsTimestamps);
+
+ Long tsAfterFullBackup = rsTimestamps.get(offlineHost);
+ assertNotNull(tsAfterFullBackup,
+ "Offline host should have timestamp recorded in trslm after full
backup");
+
+ LOG.info("Taking incremental backup (should NOT include offline RS
WALs)");
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue(checkSucceeded(incrBackupId), "Incremental backup should
succeed");
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ rsTimestamps = timestamps.get(table1);
+ assertFalse(rsTimestamps.containsKey(offlineHost),
+ "Offline host should not have a boundary after incremental");
+ }
+ }
+
+ /**
+ * Tests that WALs written to an RS after a full backup are correctly
included in the subsequent
+ * incremental backup, even if that RS has gone offline before the
incremental runs.
+ */
+ @Test
+ public void testRSGoesOfflineAfterFullBackupBeforeIncremental() throws
Exception {
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 3, 50);
+ }
+
+ String fullBackupId = fullTableBackup(tables);
+ assertTrue(checkSucceeded(fullBackupId), "Full backup should succeed");
+
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 4, 50);
+ }
+
+ HRegionServer rsBeforeStop =
cluster.getLiveRegionServerThreads().get(0).getRegionServer();
+ rsBeforeStop.getWalRoller().requestRollAll();
+ rsBeforeStop.getWalRoller().waitUntilWalRollFinished();
+ String offlineHost =
+ rsBeforeStop.getServerName().getHostname() + ":" +
rsBeforeStop.getServerName().getPort();
+
+ cluster.stopRegionServer(rsBeforeStop.getServerName());
+ Thread.sleep(5000);
+
+ String incrBackupId = incrementalTableBackup(tables);
+ assertTrue(checkSucceeded(incrBackupId), "Incremental backup should
succeed");
+
+ try (BackupSystemTable sysTable = new
BackupSystemTable(TEST_UTIL.getConnection())) {
+ Map<TableName, Map<String, Long>> timestamps =
sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
+ Map<String, Long> rsTimestamps = timestamps.get(table1);
+ assertNotNull(rsTimestamps.get(offlineHost),
+ "Offline RS should have a timestamp boundary after the incremental
backed up its WALs");
+
+ String incrBackupId2 = incrementalTableBackup(tables);
+ assertTrue(checkSucceeded(incrBackupId2), "Second incremental backup
should succeed");
+
+ timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
Review Comment:
The offline host gets re-introduced upon later incremental backups.
In the `IncrementalBackupManager`, this is the check:
```java
Long earliestTimestampToIncludeInBackup = previousTimestampMins.get(host);
boolean isInactive = earliestTimestampToIncludeInBackup != null
&& earliestTimestampToIncludeInBackup >= latestLogRoll;
```
So a host counts as inactive only if two things are true:
1. The previous backup stored a boundary for it.
2. That boundary has caught up with its last roll result.
Any host that the previous backup did not store is treated as active,
whatever its roll result says.
Replace the remaining code by this snippet as demonstration (this will make
the test fail):
```java
timestamps = sysTable.readLogTimestampMap(BACKUP_ROOT_DIR);
rsTimestamps = timestamps.get(table1);
assertFalse(rsTimestamps.containsKey(offlineHost),
"Offline RS should not have a boundary after all its WALs have been
backed up");
// The offline RS keeps its entry in the last-log-roll table, which is
never pruned. Later
// incrementals must neither restore its boundary nor back up its WALs
a second time.
Long staleLogRoll =
sysTable.readRegionServerLastLogRollResult(BACKUP_ROOT_DIR).get(offlineHost);
String incrBackupId3 = incrementalTableBackup(tables);
assertTrue(checkSucceeded(incrBackupId3), "Third incremental backup
should succeed");
Long boundaryAfterIncr3 =
sysTable.readLogTimestampMap(BACKUP_ROOT_DIR).get(table1)
.get(offlineHost);
String incrBackupId4 = incrementalTableBackup(tables);
assertTrue(checkSucceeded(incrBackupId4), "Fourth incremental backup
should succeed");
List<String> reincludedWals =
walsOfHost(sysTable.readBackupInfo(incrBackupId4), offlineHost);
assertAll(
() -> assertNull(boundaryAfterIncr3,
"Third incremental restored the pruned boundary of the offline RS
(last log roll = "
+ staleLogRoll + ")"),
() -> assertTrue(reincludedWals.isEmpty(),
"Fourth incremental backed up WALs of the offline RS again: " +
reincludedWals
+ ", first backed up in " + incrBackupId + ": " +
offlineHostWals));
}
}
private static List<String> walsOfHost(BackupInfo backupInfo, String host)
{
List<String> wals = backupInfo.getIncrBackupFileList();
if (wals == null) {
return List.of();
}
return wals.stream()
.filter(wal -> host.equals(BackupUtils.parseHostNameFromLogFile(new
Path(wal)))).toList();
}
```
A consequence is that old WALs can be backed up again, which can corrupt
data when doing a recovery.
Possible fix, use the same defintion as FullBackupTableClient (but didn't
verify this):
```
Map<String, Long> rollsBefore = readRegionServerLastLogRollResult();
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
Map<String, Long> rollsAfter = readRegionServerLastLogRollResult();
// active: rollsAfter.get(h) != null &&
!rollsAfter.get(h).equals(rollsBefore.get(h))
```
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
Some requests for javadoc or logging changes on the original PR still apply:
- https://github.com/apache/hbase/pull/7582#discussion_r2913219950
- https://github.com/apache/hbase/pull/7582#discussion_r2913240819
- https://github.com/apache/hbase/pull/7582#discussion_r2658270055
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/FullTableBackupClient.java:
##########
@@ -208,14 +214,24 @@ private void handleContinuousBackup(Admin admin) throws
IOException {
}
private void handleNonContinuousBackup(Admin admin) throws IOException {
+ Map<String, Long> previousLogRollsByHost =
backupManager.readRegionServerLastLogRollResult();
performLogRoll();
+ Map<String, Long> latestLogRollsByHost = newTimestamps;
Review Comment:
Having `performLogRoll` return the map would make this easier to follow.
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+
+/**
+ * Tests that WAL files from offline/inactive RegionServers are handled
correctly during backup.
+ * Specifically verifies that WALs from an offline RS are:
+ * <ol>
+ * <li>Backed up once in the first backup after the RS goes offline</li>
+ * <li>NOT re-backed up in subsequent backups</li>
+ * </ol>
+ */
+@Tag(LargeTests.TAG)
+public class TestBackupOfflineRS extends TestBackupBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestBackupOfflineRS.class);
+
+ @BeforeAll
+ public static void setUp() throws Exception {
+ TEST_UTIL = new HBaseTestingUtil();
+ conf1 = TEST_UTIL.getConfiguration();
+ conf1.setInt("hbase.regionserver.info.port", -1);
+ autoRestoreOnFailure = true;
+ useSecondCluster = false;
+ setUpHelper();
+ TEST_UTIL.getMiniHBaseCluster().startRegionServer();
+ TEST_UTIL.waitTableAvailable(table1);
+ }
+
+ /**
+ * Tests that when a full backup is taken while an RS is offline (with WALs
in oldlogs), the
+ * offline host's timestamps are recorded so subsequent incremental backups
don't reinclude those
+ * WALs.
+ */
+ @Test
+ public void testBackupWithOfflineRS() throws Exception {
+ LOG.info("Starting testBackupWithOfflineRS");
+
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
Review Comment:
Suggested alternative: `cluster.startRegionServerAndWait(10000);`
similar for other tests
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
Review Comment:
BackupSystemTable#writeRegionServerLogTimestamp javadoc ("timestamp is of
the last log file that was backed up") no longer matches. Stored values are now
a mix of roll timestamps and WAL creation timestamps.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -72,12 +72,46 @@ public Map<String, Long> getIncrBackupLogFileMap() throws
IOException {
LOG.info("Execute roll log procedure for incremental backup ...");
BackupUtils.logRoll(conn, backupInfo.getBackupRootDir(), conf);
- newTimestamps = readRegionServerLastLogRollResult();
+ Map<String, Long> newTimestamps = new HashMap<>();
+ Map<String, Long> latestLogRollByHost =
readRegionServerLastLogRollResult();
+ for (Map.Entry<String, Long> entry : latestLogRollByHost.entrySet()) {
+ String host = entry.getKey();
+ long latestLogRoll = entry.getValue();
+ Long earliestTimestampToIncludeInBackup =
previousTimestampMins.get(host);
+
+ boolean isInactive = earliestTimestampToIncludeInBackup != null
+ && earliestTimestampToIncludeInBackup >= latestLogRoll;
+
+ if (isInactive) {
+ LOG.debug(
+ "Skipping inactive host {} from newTimestamps (boundary={} >=
latestLogRoll={})", host,
+ earliestTimestampToIncludeInBackup, latestLogRoll);
+ } else {
+ newTimestamps.put(host, latestLogRoll);
+ }
+ }
logList = getLogFilesForNewBackup(previousTimestampMins, newTimestamps,
conf);
logList = excludeProcV2WALs(logList);
backupInfo.setIncrBackupFileList(logList);
+ // Update boundaries based on WALs that will be backed up
+ for (String logFile : logList) {
+ Path logPath = new Path(logFile);
+ String logHost = BackupUtils.parseHostFromOldLog(logPath);
+ if (logHost == null) {
+ logHost = BackupUtils.parseHostNameFromLogFile(logPath.getParent());
Review Comment:
`BackupUtils.parseHostNameFromLogFile(logPath)` covers both cases.
##########
hbase-backup/src/main/java/org/apache/hadoop/hbase/backup/impl/IncrementalBackupManager.java:
##########
@@ -217,14 +251,6 @@ private List<String> getLogFilesForNewBackup(Map<String,
Long> olderTimestamps,
resultLogFiles.add(currentLogFile);
}
- // It is possible that a host in .oldlogs is an obsolete region server
- // so newestTimestamps.get(host) here can be null.
- // Even if these logs belong to a obsolete region server, we still need
- // to include they to avoid loss of edits for backup.
- Long newTimestamp = newestTimestamps.get(host);
Review Comment:
I still argue to not delete this check, but to change it to `if
(newTimestamp != null && currentLogTS > newTimestamp)` instead. I think the
removal (as suggested in the PR) can lead to data loss. This adjusted version
fixes the original data loss issue and does not break the newly added tests.
Two things have to occur on a region server during an incremental backup to
cause data loss:
1. Two quick wall rolls. The backup's own roll creates a new WAL, C. Before
the backup finishes listing oldWALs, the region server has to roll twice more:
C → T, then T → U. Only then is T closed and eligible for archiving.
2. T is archived while C isn't. cleanOldLogs archives any closed WAL whose
regions have all flushed past its edits. If C holds edits for a region that T
doesn't touch, and that region hasn't flushed yet, T gets archived first.
Of these 2 conditions, I expect (2) to occur commonly. For (1): The window
runs from the backup's roll to the end of the oldWALs listing, and that's
normally a few seconds. Two rolls might fit in it when one of these holds:
- Heavy ingest. By default a WAL rolls when it reaches 0.5 × (2 × HDFS
block size), which is about 128 MB. A region server ingesting around 50–100
MB/s rolls every 1–3 seconds. The hourly time-based roll
(hbase.regionserver.logroll.period) is far too slow to matter.
- A slow listing. getLogFilesForNewBackup runs one listStatus per region
server directory, then a recursive listing of oldWALs. BackupLogCleaner keeps
every WAL that a backup root still needs, so with backups enabled oldWALs can
hold tens or hundreds of thousands of files. A large cluster or a slow NameNode
can stretch the window to tens of seconds.
A rare case, but one that can occur on any incremental backup on any region
server. For a large cluster with daily backups, it is bound to happen
eventually. The result is a WAL that never gets included in a backup, leading
to data loss when restoring.
I agree that simpler is better, and I wished lots of the backup code was
simpler, but in this case the complexity is worth it.
Example test case (breaks on the current PR, succeeds on the current PR +
suggested adjustment):
```java
// paste me in TestIncrementalBackupManager
/**
* WALs can be archived out of order. This test sets up the WAL
directories as an incremental
* backup would find them if, after its log roll, a newer WAL got archived
while an older one was
* still in the WALs directory. The older WAL must still end up in a
backup.
*/
@Test
public void testOutOfOrderArchivedWALDoesNotSkipOlderWAL() throws
Exception {
List<TableName> tables = List.of(table1);
HRegionServer rs = TEST_UTIL.getMiniHBaseCluster().getRegionServer(0);
ServerName serverName = rs.getServerName();
Path walRootDir = CommonFSUtils.getWALRootDir(conf1);
FileSystem fs = walRootDir.getFileSystem(conf1);
try (Connection conn = ConnectionFactory.createConnection(conf1);
BackupAdminImpl backupAdmin = new BackupAdminImpl(conn)) {
String fullBackupId =
backupAdmin.backupTables(createBackupRequest(BackupType.FULL,
tables, BACKUP_ROOT_DIR));
assertTrue(checkSucceeded(fullBackupId));
// Both test WALs are newer than any WAL that exists at this point, so
they are newer than the
// WAL reported by the log roll of the next incremental backup.
long olderWALTs = EnvironmentEdgeManager.currentTime() + 1;
long newerWALTs = olderWALTs + 1;
Path walDir =
new Path(walRootDir,
AbstractFSWALProvider.getWALDirectoryName(serverName.toString()));
Path olderWAL =
new Path(walDir, serverName.toString() +
BackupUtils.LOGNAME_SEPARATOR + olderWALTs);
Path archiveDir = new Path(walRootDir,
AbstractFSWALProvider.getWALArchiveDirectoryName(conf1,
serverName.toString()));
Path newerArchivedWAL =
new Path(archiveDir, "wal" + BackupUtils.LOGNAME_SEPARATOR +
newerWALTs);
fs.create(olderWAL).close();
fs.mkdirs(archiveDir);
fs.create(newerArchivedWAL).close();
try {
List<String> firstBackupFiles;
try (IncrementalBackupManager manager = new
IncrementalBackupManager(conn, conf1)) {
BackupInfo backupInfo = manager.createBackupInfo("backup_incr_1",
BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR, -1, -1, false,
false);
Map<String, Long> boundaries = manager.getIncrBackupLogFileMap();
manager.writeRegionServerLogTimestamp(backupInfo.getTables(),
boundaries);
firstBackupFiles = backupInfo.getIncrBackupFileList();
}
// Roll once more, so the log roll of the next backup reports a WAL
that is newer than both
// test WALs.
TEST_UTIL.waitFor(30_000, () -> EnvironmentEdgeManager.currentTime()
> newerWALTs);
rs.getWalRoller().requestRollAll();
rs.getWalRoller().waitUntilWalRollFinished();
List<String> secondBackupFiles;
try (IncrementalBackupManager manager = new
IncrementalBackupManager(conn, conf1)) {
BackupInfo backupInfo = manager.createBackupInfo("backup_incr_2",
BackupType.INCREMENTAL, tables, BACKUP_ROOT_DIR, -1, -1, false,
false);
manager.getIncrBackupLogFileMap();
secondBackupFiles = backupInfo.getIncrBackupFileList();
}
assertTrue(
firstBackupFiles.contains(olderWAL.toString())
|| secondBackupFiles.contains(olderWAL.toString()),
"WAL " + olderWAL + " was not included in any backup. First
backup: " + firstBackupFiles
+ ", second backup: " + secondBackupFiles);
} finally {
fs.delete(olderWAL, false);
fs.delete(newerArchivedWAL, false);
}
}
}
```
##########
hbase-backup/src/test/java/org/apache/hadoop/hbase/backup/TestBackupOfflineRS.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.backup;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.util.List;
+import java.util.Map;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.SingleProcessHBaseCluster;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.backup.impl.BackupSystemTable;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.ConnectionFactory;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
+
+/**
+ * Tests that WAL files from offline/inactive RegionServers are handled
correctly during backup.
+ * Specifically verifies that WALs from an offline RS are:
+ * <ol>
+ * <li>Backed up once in the first backup after the RS goes offline</li>
+ * <li>NOT re-backed up in subsequent backups</li>
+ * </ol>
+ */
+@Tag(LargeTests.TAG)
+public class TestBackupOfflineRS extends TestBackupBase {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TestBackupOfflineRS.class);
+
+ @BeforeAll
+ public static void setUp() throws Exception {
+ TEST_UTIL = new HBaseTestingUtil();
+ conf1 = TEST_UTIL.getConfiguration();
+ conf1.setInt("hbase.regionserver.info.port", -1);
+ autoRestoreOnFailure = true;
+ useSecondCluster = false;
+ setUpHelper();
+ TEST_UTIL.getMiniHBaseCluster().startRegionServer();
+ TEST_UTIL.waitTableAvailable(table1);
+ }
+
+ /**
+ * Tests that when a full backup is taken while an RS is offline (with WALs
in oldlogs), the
+ * offline host's timestamps are recorded so subsequent incremental backups
don't reinclude those
+ * WALs.
+ */
+ @Test
+ public void testBackupWithOfflineRS() throws Exception {
+ LOG.info("Starting testBackupWithOfflineRS");
+
+ SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
+ List<TableName> tables = Lists.newArrayList(table1);
+
+ if (cluster.getNumLiveRegionServers() < 2) {
+ cluster.startRegionServer();
+ Thread.sleep(2000);
+ }
+
+ LOG.info("Inserting data to generate WAL entries");
+ try (Connection conn = ConnectionFactory.createConnection(conf1)) {
+ insertIntoTable(conn, table1, famName, 2, 100);
+ }
+
+ int rsToStop = 0;
+ HRegionServer rsBeforeStop = cluster.getRegionServer(rsToStop);
Review Comment:
Suggested alternative (this also removes the `sleep`):
```
HRegionServer rsBeforeStop =
cluster.getLiveRegionServerThreads().get(0).getRegionServer();
stopRegionServerAndWait(rsBeforeStop.getServerName());
```
with
```java
/**
* Stops the RS and waits until its WALs are archived and its regions are
open on other servers.
*/
private static void stopRegionServerAndWait(ServerName serverName) throws
Exception {
SingleProcessHBaseCluster cluster = TEST_UTIL.getMiniHBaseCluster();
HMaster master = cluster.getMaster();
cluster.stopRegionServer(serverName);
// On a graceful stop, the RS moves its WALs to oldWALs before its
thread exits.
cluster.waitForRegionServerToStop(serverName, 60_000);
// The master reassigns the regions of the stopped RS in a
ServerCrashProcedure.
TEST_UTIL.waitFor(60_000,
() ->
master.getProcedures().stream().filter(ServerCrashProcedure.class::isInstance)
.map(ServerCrashProcedure.class::cast)
.anyMatch(scp -> scp.getServerName().equals(serverName) &&
scp.isFinished()));
TEST_UTIL.waitUntilNoRegionsInTransition(60_000);
}
```
(remove the sleeps in the other tests as well)
--
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]
