Copilot commented on code in PR #8126:
URL: https://github.com/apache/hbase/pull/8126#discussion_r3136218192
##########
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:
assertEquals(expected, actual, ...) has the expected/actual arguments
reversed (metaState.getState() vs State.OPEN), which makes assertion failures
harder to understand. Please swap them so the expected enum constant comes
first.
##########
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:
The arguments to assertEquals are swapped (actual first, expected second).
This doesn't change pass/fail, but it produces misleading failure diagnostics.
Please put the constant RegionState.State.OPEN as the expected value and
metaState.getState() as the actual.
##########
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?");
int version1 = ZKUtil.checkExists(zkw, tasknode);
Review Comment:
This assertEquals has expected/actual swapped (tot_mgr_resubmit_failed.sum()
vs 0). While equality is symmetric, putting the constant expected value first
makes failures much easier to diagnose ("expected: <0> but was: <...>").
##########
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);
+ }
Review Comment:
The ERROR_OUT/ERROR_COUNTER/FIRST_TIME_ERROR statics are not reset in
setUpBeforeClass/tearDownAfterClass. Since this base class is shared by
multiple concrete test classes, the first run can leave FIRST_TIME_ERROR=false
and a non-zero counter, making later classes order-dependent/flaky. Recommend
explicitly resetting these statics (and any other shared state) when starting
the mini cluster.
##########
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:
In JUnit 5, the conventional parameter order is assertEquals(expected,
actual, ...). Here the order is reversed, which makes failure output confusing
(expected becomes the dynamic list size). Consider swapping the arguments so
SLAVES/0 are the expected values and listUnknownServers().size() is the actual
value.
--
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]
