Copilot commented on code in PR #8125:
URL: https://github.com/apache/hbase/pull/8125#discussion_r3137749276
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestListTablesByState.java:
##########
@@ -72,21 +69,21 @@ public void before() throws Exception {
public void testListTableNamesByState() throws Exception {
ADMIN.createTable(TABLE_DESC);
ADMIN.disableTable(TABLE);
- Assert.assertEquals(ADMIN.listTableNamesByState(false).get(0), TABLE);
+ assertEquals(ADMIN.listTableNamesByState(false).get(0), TABLE);
ADMIN.enableTable(TABLE);
- Assert.assertEquals(ADMIN.listTableNamesByState(true).get(0), TABLE);
+ assertEquals(ADMIN.listTableNamesByState(true).get(0), TABLE);
Review Comment:
JUnit5 assertEquals signature is (expected, actual). These assertions
currently pass (equality is symmetric) but will produce misleading
expected/actual values on failure. Swap the arguments so TABLE is the expected
value.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestSplitLogManager.java:
##########
@@ -342,8 +336,7 @@ public long eval() {
return (tot_mgr_resubmit.sum() + tot_mgr_resubmit_failed.sum());
}
}, 0, 1, 5 * 60000); // wait long enough
- Assert.assertEquals("Could not run test. Lost ZK connection?", 0,
- tot_mgr_resubmit_failed.sum());
+ assertEquals(tot_mgr_resubmit_failed.sum(), 0, "Could not run test. Lost
ZK connection?");
Review Comment:
JUnit5 assertEquals signature is (expected, actual, message). The arguments
are swapped here; use 0 as the expected value and tot_mgr_resubmit_failed.sum()
as the actual value to avoid confusing failure output.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMetaShutdownHandler.java:
##########
@@ -99,8 +94,8 @@ public void testExpireMetaRegionServer() throws Exception {
metaServerName =
regionStates.getRegionServerOfRegion(HRegionInfo.FIRST_META_REGIONINFO);
}
RegionState metaState =
MetaTableLocator.getMetaRegionState(master.getZooKeeper());
- assertEquals("Wrong state for meta!", RegionState.State.OPEN,
metaState.getState());
- assertNotEquals("Meta is on master!", metaServerName,
master.getServerName());
+ assertEquals(metaState.getState(), RegionState.State.OPEN, "Wrong state
for meta!");
+ assertNotEquals(master.getServerName(), metaServerName, "Meta is on
master!");
Review Comment:
JUnit5 assertEquals signature is (expected, actual, message). The
expected/actual arguments are swapped here; use RegionState.State.OPEN as
expected and metaState.getState() as actual so assertion failures are reported
correctly.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestListTablesByState.java:
##########
@@ -72,21 +69,21 @@ public void before() throws Exception {
public void testListTableNamesByState() throws Exception {
ADMIN.createTable(TABLE_DESC);
ADMIN.disableTable(TABLE);
- Assert.assertEquals(ADMIN.listTableNamesByState(false).get(0), TABLE);
+ assertEquals(ADMIN.listTableNamesByState(false).get(0), TABLE);
ADMIN.enableTable(TABLE);
- Assert.assertEquals(ADMIN.listTableNamesByState(true).get(0), TABLE);
+ assertEquals(ADMIN.listTableNamesByState(true).get(0), TABLE);
}
@Test
public void testListTableDescriptorByState() throws Exception {
ADMIN.createTable(TABLE_DESC);
ADMIN.disableTable(TABLE);
-
Assert.assertEquals(ADMIN.listTableDescriptorsByState(false).get(0).getTableName(),
TABLE);
+
assertEquals(ADMIN.listTableDescriptorsByState(false).get(0).getTableName(),
TABLE);
ADMIN.enableTable(TABLE);
-
Assert.assertEquals(ADMIN.listTableDescriptorsByState(true).get(0).getTableName(),
TABLE);
+
assertEquals(ADMIN.listTableDescriptorsByState(true).get(0).getTableName(),
TABLE);
Review Comment:
JUnit5 assertEquals signature is (expected, actual). These assertions
currently have the arguments swapped, which makes failure output confusing.
Swap argument order so TABLE is expected and the list-derived value is actual.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMetaShutdownHandler.java:
##########
@@ -123,15 +118,16 @@ public boolean evaluate() throws Exception {
LOG.info("Past wait on RIT");
TEST_UTIL.waitUntilNoRegionsInTransition(60000);
// Now, make sure meta is assigned
- assertTrue("Meta should be assigned",
- regionStates.isRegionOnline(HRegionInfo.FIRST_META_REGIONINFO));
+ assertTrue(regionStates.isRegionOnline(HRegionInfo.FIRST_META_REGIONINFO),
+ "Meta should be assigned");
// Now, make sure meta is registered in zk
metaState = MetaTableLocator.getMetaRegionState(master.getZooKeeper());
- assertEquals("Meta should not be in transition", RegionState.State.OPEN,
metaState.getState());
- assertEquals("Meta should be assigned", metaState.getServerName(),
- regionStates.getRegionServerOfRegion(HRegionInfo.FIRST_META_REGIONINFO));
- assertNotEquals("Meta should be assigned on a different server",
metaState.getServerName(),
- metaServerName);
+ assertEquals(RegionState.State.OPEN, metaState.getState(), "Meta should
not be in transition");
+ assertEquals(metaState.getServerName(),
+ regionStates.getRegionServerOfRegion(HRegionInfo.FIRST_META_REGIONINFO),
+ "Meta should be assigned");
Review Comment:
JUnit5 assertEquals signature is (expected, actual, message). Here
expected/actual are swapped; the assignment manager’s server (expected) should
be first, and metaState.getServerName() (actual) second, to keep failure output
meaningful.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java:
##########
@@ -194,8 +185,8 @@ public void testMetaInTransitionWhenMasterFailover() throws
Exception {
// meta should remain where it was
RegionState metaState =
MetaTableLocator.getMetaRegionState(hrs.getZooKeeper());
- assertEquals("hbase:meta should be online on RS",
metaState.getServerName(), metaServerName);
- assertEquals("hbase:meta should be online on RS", State.OPEN,
metaState.getState());
+ assertEquals(metaServerName, metaState.getServerName(), "hbase:meta
should be online on RS");
+ assertEquals(metaState.getState(), State.OPEN, "hbase:meta should be
online on RS");
Review Comment:
JUnit5 assertEquals signature is (expected, actual, message). The arguments
are swapped here; use State.OPEN as expected and metaState.getState() as actual
to avoid confusing failures.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestUnknownServers.java:
##########
@@ -55,12 +52,12 @@ public static void setUpBeforeClass() throws Exception {
@Test
public void testListUnknownServers() throws Exception {
- Assert.assertEquals(ADMIN.listUnknownServers().size(), SLAVES);
+ assertEquals(ADMIN.listUnknownServers().size(), SLAVES);
IS_UNKNOWN_SERVER = false;
- Assert.assertEquals(ADMIN.listUnknownServers().size(), 0);
+ assertEquals(ADMIN.listUnknownServers().size(), 0);
Review Comment:
JUnit5 assertEquals signature is (expected, actual). Here the arguments are
swapped, so failures will report the wrong expected/actual values. Swap to
assertEquals(SLAVES, ADMIN.listUnknownServers().size()) and assertEquals(0,
ADMIN.listUnknownServers().size()).
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterFailover.java:
##########
@@ -206,8 +197,8 @@ public void testMetaInTransitionWhenMasterFailover() throws
Exception {
// ensure meta is still deployed on RS
metaState =
MetaTableLocator.getMetaRegionState(activeMaster.getZooKeeper());
- assertEquals("hbase:meta should be online on RS",
metaState.getServerName(), metaServerName);
- assertEquals("hbase:meta should be online on RS", State.OPEN,
metaState.getState());
+ assertEquals(metaServerName, metaState.getServerName(), "hbase:meta
should be online on RS");
+ assertEquals(metaState.getState(), State.OPEN, "hbase:meta should be
online on RS");
Review Comment:
Same issue as above: JUnit5 assertEquals is (expected, actual, message).
Swap arguments to assertEquals(State.OPEN, metaState.getState(), ...) so
expected/actual are reported correctly on failure.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/AbstractTestMasterRegionMutation.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * 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.master;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import java.io.IOException;
+import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.fs.FileSystem;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.hbase.HBaseTestingUtility;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.MiniHBaseCluster;
+import org.apache.hadoop.hbase.RegionTooBusyException;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.StartMiniClusterOption;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.Mutation;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.master.hbck.HbckChore;
+import org.apache.hadoop.hbase.master.hbck.HbckReport;
+import org.apache.hadoop.hbase.master.region.MasterRegionFactory;
+import org.apache.hadoop.hbase.regionserver.HRegion;
+import org.apache.hadoop.hbase.regionserver.HRegionFileSystem;
+import org.apache.hadoop.hbase.regionserver.HRegionServer;
+import org.apache.hadoop.hbase.regionserver.OperationStatus;
+import org.apache.hadoop.hbase.regionserver.RegionServerServices;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.apache.hadoop.hbase.wal.WAL;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hadoop.hbase.shaded.protobuf.generated.ProcedureProtos;
+
+public abstract class AbstractTestMasterRegionMutation {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(AbstractTestMasterRegionMutation.class);
+
+ protected static final HBaseTestingUtility TEST_UTIL = new
HBaseTestingUtility();
+ protected static ServerName rs0;
+
+ protected static final AtomicBoolean ERROR_OUT = new AtomicBoolean(false);
+ protected static final AtomicInteger ERROR_COUNTER = new AtomicInteger(0);
+ protected static final AtomicBoolean FIRST_TIME_ERROR = new
AtomicBoolean(true);
+
+ protected static void setUpBeforeClass(int numMasters, Class<? extends
HRegion> regionImplClass)
+ throws Exception {
+ TEST_UTIL.getConfiguration().setClass(HConstants.REGION_IMPL,
regionImplClass, HRegion.class);
+ StartMiniClusterOption.Builder builder = StartMiniClusterOption.builder();
+ builder.numMasters(numMasters).numRegionServers(3);
+ TEST_UTIL.startMiniCluster(builder.build());
+ MiniHBaseCluster cluster = TEST_UTIL.getHBaseCluster();
+ rs0 = cluster.getRegionServer(0).getServerName();
+ TEST_UTIL.getAdmin().balancerSwitch(false, true);
+ }
+
+ protected static void tearDownAfterClass() throws Exception {
+ TEST_UTIL.shutdownMiniCluster();
+ }
+
+ @BeforeEach
+ public void setUp(TestInfo testInfo) throws Exception {
+ final TableName tableName =
TableName.valueOf(testInfo.getTestMethod().get().getName());
+ TableDescriptor tableDesc = TableDescriptorBuilder.newBuilder(tableName)
+ .setColumnFamily(ColumnFamilyDescriptorBuilder.of("fam1")).build();
+ int startKey = 0;
+ int endKey = 80000;
+ TEST_UTIL.getAdmin().createTable(tableDesc, Bytes.toBytes(startKey),
Bytes.toBytes(endKey), 9);
+ }
+
+ @Test
+ public void testMasterRegionMutations() throws Exception {
+ HbckChore hbckChore = new
HbckChore(TEST_UTIL.getHBaseCluster().getMaster());
+ MiniHBaseCluster cluster = TEST_UTIL.getHBaseCluster();
+
+ HRegionServer hRegionServer0 = cluster.getRegionServer(0);
+ HRegionServer hRegionServer1 = cluster.getRegionServer(1);
+ HRegionServer hRegionServer2 = cluster.getRegionServer(2);
+ int numRegions0 = hRegionServer0.getNumberOfOnlineRegions();
+ int numRegions1 = hRegionServer1.getNumberOfOnlineRegions();
+ int numRegions2 = hRegionServer2.getNumberOfOnlineRegions();
+
+ hbckChore.choreForTesting();
+ HbckReport hbckReport = hbckChore.getLastReport();
+ assertEquals(0, hbckReport.getInconsistentRegions().size());
+ assertEquals(0, hbckReport.getOrphanRegionsOnFS().size());
+ assertEquals(0, hbckReport.getOrphanRegionsOnRS().size());
+
+ // procedure state store update encounters retriable error, master abort
is not required
+ ERROR_OUT.set(true);
+
+ // move one region from server 1 to server 0
+ TEST_UTIL.getAdmin()
+
.move(hRegionServer1.getRegions().get(0).getRegionInfo().getEncodedNameAsBytes(),
rs0);
+
+ // procedure state store update encounters retriable error, however all
retries are exhausted.
+ // This leads to the trigger of active master abort and hence master
failover.
+ ERROR_OUT.set(true);
+
+ // move one region from server 2 to server 0
+ TEST_UTIL.getAdmin()
+
.move(hRegionServer2.getRegions().get(0).getRegionInfo().getEncodedNameAsBytes(),
rs0);
+
+ HMaster master = TEST_UTIL.getHBaseCluster().getMaster();
+
+ // Ensure:
+ // 1. num of regions before and after master abort remain same
+ // 2. all procedures are successfully completed
+ TEST_UTIL.waitFor(5000, 1000, () -> {
+ LOG.info("numRegions0: {} , numRegions1: {} , numRegions2: {}",
numRegions0, numRegions1,
+ numRegions2);
+ LOG.info("Online regions - server0 : {} , server1: {} , server2: {}",
+ cluster.getRegionServer(0).getNumberOfOnlineRegions(),
+ cluster.getRegionServer(1).getNumberOfOnlineRegions(),
+ cluster.getRegionServer(2).getNumberOfOnlineRegions());
+ LOG.info("Num of successfully completed procedures: {} , num of all
procedures: {}",
+ master.getMasterProcedureExecutor().getProcedures().stream()
+ .filter(masterProcedureEnvProcedure ->
masterProcedureEnvProcedure.getState()
+ == ProcedureProtos.ProcedureState.SUCCESS)
+ .count(),
+ master.getMasterProcedureExecutor().getProcedures().size());
+ return (numRegions0 + numRegions1 + numRegions2)
+ == (cluster.getRegionServer(0).getNumberOfOnlineRegions()
+ + cluster.getRegionServer(1).getNumberOfOnlineRegions()
+ + cluster.getRegionServer(2).getNumberOfOnlineRegions())
+ && master.getMasterProcedureExecutor().getProcedures().stream()
+ .filter(masterProcedureEnvProcedure ->
masterProcedureEnvProcedure.getState()
+ == ProcedureProtos.ProcedureState.SUCCESS)
+ .count() ==
master.getMasterProcedureExecutor().getProcedures().size();
+ });
+
+ // Ensure we have no inconsistent regions
+ TEST_UTIL.waitFor(5000, 1000, () -> {
+ HbckChore hbck = new HbckChore(TEST_UTIL.getHBaseCluster().getMaster());
+ hbck.choreForTesting();
+ HbckReport report = hbck.getLastReport();
+ return report.getInconsistentRegions().isEmpty() &&
report.getOrphanRegionsOnFS().isEmpty()
+ && report.getOrphanRegionsOnRS().isEmpty();
+ });
+
+ }
+
+ public static class TestRegion extends HRegion {
+
+ public TestRegion(Path tableDir, WAL wal, FileSystem fs, Configuration
confParam,
+ RegionInfo regionInfo, TableDescriptor htd, RegionServerServices
rsServices) {
+ super(tableDir, wal, fs, confParam, regionInfo, htd, rsServices);
+ }
+
+ public TestRegion(HRegionFileSystem fs, WAL wal, Configuration confParam,
TableDescriptor htd,
+ RegionServerServices rsServices) {
+ super(fs, wal, confParam, htd, rsServices);
+ }
+
+ @Override
+ public OperationStatus[] batchMutate(Mutation[] mutations, boolean atomic,
long nonceGroup,
+ long nonce) throws IOException {
+ if (
+
MasterRegionFactory.TABLE_NAME.equals(getTableDescriptor().getTableName())
+ && ERROR_OUT.get()
+ ) {
+ // First time errors are recovered with enough retries
+ if (FIRST_TIME_ERROR.get() && ERROR_COUNTER.getAndIncrement() == 5) {
+ ERROR_OUT.set(false);
+ ERROR_COUNTER.set(0);
+ FIRST_TIME_ERROR.set(false);
+ return super.batchMutate(mutations, atomic, nonceGroup, nonce);
+ }
+ // Second time errors are not recovered with enough retries, leading
to master abort
+ if (!FIRST_TIME_ERROR.get() && ERROR_COUNTER.getAndIncrement() == 8) {
+ ERROR_OUT.set(false);
+ ERROR_COUNTER.set(0);
+ }
+ throw new RegionTooBusyException("test error...");
+ }
+ return super.batchMutate(mutations, atomic, nonceGroup, nonce);
+ }
+ }
Review Comment:
AbstractTestMasterRegionMutation declares an inner TestRegion class (lines
166-201) but it is not referenced anywhere (subclasses pass their own
TestRegion implementations into setUpBeforeClass). Consider removing this
unused nested class, or have subclasses reuse it where applicable, to avoid
dead/duplicated test code.
```suggestion
```
--
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]
