Copilot commented on code in PR #8113:
URL: https://github.com/apache/hbase/pull/8113#discussion_r3124813324
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterQosFunction.java:
##########
@@ -46,23 +44,20 @@
import org.apache.hadoop.hbase.shaded.protobuf.generated.HBaseProtos;
import
org.apache.hadoop.hbase.shaded.protobuf.generated.RegionServerStatusProtos;
-@Category({ MasterTests.class, SmallTests.class })
+@Tag(MasterTests.TAG)
+@Tag(SmallTests.TAG)
public class TestMasterQosFunction extends QosTestHelper {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestMasterQosFunction.class);
-
private Configuration conf;
private RSRpcServices rpcServices;
private AnnotationReadingPriorityFunction qosFunction;
- @Before
+ @BeforeEach
public void setUp() {
conf = HBaseConfiguration.create();
rpcServices = Mockito.mock(MasterRpcServices.class);
when(rpcServices.getConfiguration()).thenReturn(conf);
- qosFunction = new MasterAnnotationReadingPriorityFunction(rpcServices,
MasterRpcServices.class);
+ qosFunction = new MasterAnnotationReadingPriorityFunction(rpcServices);
}
Review Comment:
`MasterAnnotationReadingPriorityFunction(RSRpcServices)` delegates to
`AnnotationReadingPriorityFunction(rpcServices, rpcServices.getClass())`. Since
`rpcServices` here is a Mockito mock, `rpcServices.getClass()` is the generated
mock class (which won’t carry the `@QosPriority` annotations from
`MasterRpcServices`), so the annotated QoS map will be empty and this test can
become incorrect/flaky. Prefer using the 2-arg constructor and pass
`MasterRpcServices.class` explicitly (or use a real `MasterRpcServices` class
for annotation scanning).
##########
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();
+ }
Review Comment:
The error-injection state (`ERROR_OUT`, `ERROR_COUNTER`, `FIRST_TIME_ERROR`)
is `static` in this base class and is used by both `TestMasterRegionMutation1`
and `TestMasterRegionMutation2` via their `TestRegion` implementations. Because
the flags are mutated during the test (e.g., `FIRST_TIME_ERROR` is set to
`false`), running one test class will change the initial conditions for the
other, leading to cross-test interference and potentially different behavior
than intended. Reset these flags in `setUpBeforeClass`/`@BeforeEach` (and/or
make them per-test-class rather than shared static state) so each test class
starts from a clean state.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestRollingRestart.java:
##########
@@ -59,24 +57,29 @@
/**
* Tests the restarting of everything as done during rolling restarts.
*/
-@RunWith(Parameterized.class)
-@Category({ MasterTests.class, LargeTests.class })
+@Tag(MasterTests.TAG)
+@Tag(LargeTests.TAG)
+@HBaseParameterizedTestTemplate(name = "{index}: splitWALCoordinatedByZK={0}")
public class TestRollingRestart {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestRollingRestart.class);
-
private static final Logger LOG =
LoggerFactory.getLogger(TestRollingRestart.class);
private static HBaseTestingUtility TEST_UTIL;
- @Rule
- public TestName name = new TestName();
- @Parameterized.Parameter
- public boolean splitWALCoordinatedByZK;
+ private String testMethodName;
+
+ @BeforeEach
+ public void setTestMethod(TestInfo testInfo) {
+ testMethodName = testInfo.getTestMethod().get().getName();
+ }
Review Comment:
`testMethodName` is derived only from
`testInfo.getTestMethod().get().getName()`, which is identical for every
`@TestTemplate` invocation. With `@HBaseParameterizedTestTemplate` there are
multiple invocations per method, so using only the method name can cause
collisions (e.g., table names / test dirs) between parameter runs and lead to
flaky behavior under parallel execution or leftover state. Consider
incorporating `testInfo.getDisplayName()` (or the parameter value) when
building `testMethodName`, similar to other parameterized tests in the codebase.
##########
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:
The `assertEquals` arguments are reversed here. In JUnit Jupiter the
convention/signature is `assertEquals(expected, actual, message)`, so this
should be `assertEquals(0, tot_mgr_resubmit_failed.sum(), ...)` to keep failure
output readable and consistent.
--
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]
