Copilot commented on code in PR #8099:
URL: https://github.com/apache/hbase/pull/8099#discussion_r3109020274
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRawAsyncTableScan.java:
##########
@@ -32,37 +32,31 @@
import java.util.List;
import java.util.function.Supplier;
import java.util.stream.Collectors;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
+import java.util.stream.Stream;
+import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.client.trace.StringTraceRenderer;
import org.apache.hadoop.hbase.testclassification.ClientTests;
import org.apache.hadoop.hbase.testclassification.LargeTests;
import org.hamcrest.Matcher;
-import org.junit.ClassRule;
-import org.junit.experimental.categories.Category;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameter;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.provider.Arguments;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
-@Category({ LargeTests.class, ClientTests.class })
+@Tag(LargeTests.TAG)
+@Tag(ClientTests.TAG)
+@HBaseParameterizedTestTemplate(name = "{index}: scan={0}")
public class TestRawAsyncTableScan extends AbstractTestAsyncTableScan {
private static final Logger logger =
LoggerFactory.getLogger(TestRawAsyncTableScan.class);
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestRawAsyncTableScan.class);
+ private Supplier<Scan> scanCreater;
- @Parameter(0)
- public String scanType;
-
- @Parameter(1)
- public Supplier<Scan> scanCreater;
+ // scanType is only for displaying
+ public TestRawAsyncTableScan(String scanType, Supplier<Scan> scanCreater) {
+ this.scanCreater = scanCreater;
+ }
Review Comment:
The field/parameter name `scanCreater` appears to be a misspelling and is
inconsistent with other classes in this PR that use `scanCreator`. Renaming to
`scanCreator` will improve readability and reduce confusion.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableScan.java:
##########
@@ -32,37 +32,31 @@
import java.util.concurrent.ForkJoinPool;
import java.util.function.Supplier;
import java.util.stream.Collectors;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
+import java.util.stream.Stream;
+import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.client.trace.StringTraceRenderer;
import org.apache.hadoop.hbase.testclassification.ClientTests;
import org.apache.hadoop.hbase.testclassification.LargeTests;
import org.hamcrest.Matcher;
-import org.junit.ClassRule;
-import org.junit.experimental.categories.Category;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameter;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.provider.Arguments;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
-@Category({ LargeTests.class, ClientTests.class })
+@Tag(LargeTests.TAG)
+@Tag(ClientTests.TAG)
+@HBaseParameterizedTestTemplate(name = "{index}: scan={0}")
public class TestAsyncTableScan extends AbstractTestAsyncTableScan {
private static final Logger logger =
LoggerFactory.getLogger(TestAsyncTableScan.class);
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestAsyncTableScan.class);
+ private Supplier<Scan> scanCreater;
- @Parameter(0)
- public String scanType;
-
- @Parameter(1)
- public Supplier<Scan> scanCreater;
+ // scanType is only for displaying
+ public TestAsyncTableScan(String scanType, Supplier<Scan> scanCreater) {
+ this.scanCreater = scanCreater;
+ }
Review Comment:
Same as in `TestRawAsyncTableScan`: `scanCreater` looks like a misspelling.
Rename to `scanCreator` for consistency with
`TestAsyncTableScanner`/`TestAsyncTableScanAll` and to avoid propagating the
typo.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableScanner.java:
##########
@@ -29,46 +29,37 @@
import io.opentelemetry.sdk.trace.data.SpanData;
import java.util.ArrayList;
import java.util.List;
-import java.util.concurrent.ForkJoinPool;
import java.util.function.Supplier;
import java.util.stream.Collectors;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
+import java.util.stream.Stream;
+import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.client.trace.StringTraceRenderer;
import org.apache.hadoop.hbase.testclassification.ClientTests;
import org.apache.hadoop.hbase.testclassification.LargeTests;
import org.hamcrest.Matcher;
-import org.junit.ClassRule;
-import org.junit.experimental.categories.Category;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameter;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.params.provider.Arguments;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Parameterized.class)
-@Category({ LargeTests.class, ClientTests.class })
+@Tag(LargeTests.TAG)
+@Tag(ClientTests.TAG)
+@HBaseParameterizedTestTemplate(name = "{index}: table={0}, scan={2}")
public class TestAsyncTableScanner extends AbstractTestAsyncTableScan {
private static final Logger logger =
LoggerFactory.getLogger(TestAsyncTableScanner.class);
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestAsyncTableScanner.class);
+ private Supplier<AsyncTable<?>> getTable;
- @Parameter(0)
- public String tableType;
+ private Supplier<Scan> scanCreator;
- @Parameter(1)
- public Supplier<AsyncTable<?>> getTable;
-
- @Parameter(2)
- public String scanType;
-
- @Parameter(3)
- public Supplier<Scan> scanCreator;
+ // tableType and scanType are only for displaying
+ public TestAsyncTableScanner(String tableType, Supplier<AsyncTable<?>>
getTable, String scanType,
+ Supplier<Scan> scanCreator) {
+ this.getTable = getTable;
+ this.scanCreator = scanCreator;
+ }
Review Comment:
These constructor-initialized fields can be made `final` to document
immutability of the test configuration per invocation and prevent accidental
reassignment later.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/AbstractTestAsyncTableScan.java:
##########
@@ -44,82 +45,66 @@
import java.util.stream.Collectors;
import java.util.stream.IntStream;
import java.util.stream.Stream;
-import org.apache.hadoop.conf.Configuration;
-import org.apache.hadoop.hbase.ConnectionRule;
import org.apache.hadoop.hbase.HBaseTestingUtil;
import org.apache.hadoop.hbase.MatcherPredicate;
-import org.apache.hadoop.hbase.MiniClusterRule;
-import org.apache.hadoop.hbase.StartTestingClusterOption;
import org.apache.hadoop.hbase.TableName;
-import org.apache.hadoop.hbase.Waiter;
import org.apache.hadoop.hbase.ipc.RemoteWithExtrasException;
import org.apache.hadoop.hbase.regionserver.NoSuchColumnFamilyException;
import org.apache.hadoop.hbase.trace.HBaseSemanticAttributes;
-import org.apache.hadoop.hbase.trace.OpenTelemetryClassRule;
-import org.apache.hadoop.hbase.trace.OpenTelemetryTestRule;
import org.apache.hadoop.hbase.trace.TraceUtil;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.JVMClusterUtil;
import org.apache.hadoop.hbase.util.Pair;
import org.hamcrest.Matcher;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.ExternalResource;
-import org.junit.rules.RuleChain;
-import org.junit.rules.TestName;
-import org.junit.rules.TestRule;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.provider.Arguments;
-public abstract class AbstractTestAsyncTableScan {
+import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
- protected static final OpenTelemetryClassRule OTEL_CLASS_RULE =
OpenTelemetryClassRule.create();
+public abstract class AbstractTestAsyncTableScan {
- private static Configuration createConfiguration() {
- Configuration conf = new Configuration();
- // Disable directory sharing to prevent race conditions when tests run in
parallel.
- // Each test instance gets its own isolated directories to avoid one
test's tearDown()
- // deleting directories another parallel test is still using.
- conf.setBoolean("hbase.test.disable-directory-sharing", true);
- return conf;
- }
+ @RegisterExtension
+ protected static final OpenTelemetryExtension OTEL_EXT =
OpenTelemetryExtension.create();
- protected static final MiniClusterRule MINI_CLUSTER_RULE =
- MiniClusterRule.newBuilder().setConfiguration(createConfiguration())
-
.setMiniClusterOption(StartTestingClusterOption.builder().numWorkers(3).build()).build();
+ protected static final HBaseTestingUtil UTIL = new HBaseTestingUtil();
- protected static final ConnectionRule CONN_RULE =
-
ConnectionRule.createAsyncConnectionRule(MINI_CLUSTER_RULE::createAsyncConnection);
+ protected static AsyncConnection CONN;
- private static final class Setup extends ExternalResource {
- @Override
- protected void before() throws Throwable {
- final HBaseTestingUtil testingUtil =
MINI_CLUSTER_RULE.getTestingUtility();
- final AsyncConnection conn = CONN_RULE.getAsyncConnection();
+ protected String methodName;
- byte[][] splitKeys = new byte[8][];
- for (int i = 111; i < 999; i += 111) {
- splitKeys[i / 111 - 1] = Bytes.toBytes(String.format("%03d", i));
- }
- testingUtil.createTable(TABLE_NAME, FAMILY, splitKeys);
- testingUtil.waitTableAvailable(TABLE_NAME);
- conn.getTable(TABLE_NAME)
- .putAll(IntStream.range(0, COUNT)
- .mapToObj(i -> new Put(Bytes.toBytes(String.format("%03d", i)))
- .addColumn(FAMILY, CQ1, Bytes.toBytes(i)).addColumn(FAMILY, CQ2,
Bytes.toBytes(i * i)))
- .collect(Collectors.toList()))
- .get();
+ @BeforeAll
+ public static void setUpBeforeClass() throws Exception {
+ UTIL.startMiniCluster(3);
+ byte[][] splitKeys = new byte[8][];
+ for (int i = 111; i < 999; i += 111) {
+ splitKeys[i / 111 - 1] = Bytes.toBytes(String.format("%03d", i));
}
+ UTIL.createTable(TABLE_NAME, FAMILY, splitKeys);
+ UTIL.waitTableAvailable(TABLE_NAME);
+ try (Table table = UTIL.getConnection().getTable(TABLE_NAME)) {
+ table.put(IntStream.range(0, COUNT)
+ .mapToObj(i -> new Put(Bytes.toBytes(String.format("%03d", i)))
+ .addColumn(FAMILY, CQ1, Bytes.toBytes(i)).addColumn(FAMILY, CQ2,
Bytes.toBytes(i * i)))
+ .collect(Collectors.toList()));
+ }
+ CONN =
ConnectionFactory.createAsyncConnection(UTIL.getConfiguration()).get();
}
- @ClassRule
- public static final TestRule classRule = RuleChain.outerRule(OTEL_CLASS_RULE)
- .around(MINI_CLUSTER_RULE).around(CONN_RULE).around(new Setup());
-
- @Rule
- public final OpenTelemetryTestRule otelTestRule = new
OpenTelemetryTestRule(OTEL_CLASS_RULE);
+ @AfterAll
+ public static void tearDownAfterClass() throws Exception {
+ Closeables.close(CONN, true);
+ UTIL.shutdownMiniCluster();
+ }
- @Rule
- public final TestName testName = new TestName();
+ @BeforeEach
+ public void setUp(TestInfo testInfo) {
+ methodName = testInfo.getTestMethod().get().getName();
Review Comment:
`methodName` is set to the Java method name only. With `@TestTemplate`
parameterized invocations, multiple runs of the same test method will typically
share this value, which can cause span name collisions (e.g.,
`waitForSpan(hasName(methodName))` may match a span from a different
invocation). Consider using `testInfo.getDisplayName()` (or storing both method
name + display name) and using that consistently for `TraceUtil.trace(...)` and
span matchers so each invocation has a unique parent span name.
```suggestion
methodName = testInfo.getDisplayName();
```
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/AbstractTestAsyncTableScan.java:
##########
@@ -211,25 +193,22 @@ protected final List<Result>
convertFromBatchResult(List<Result> results) {
}
protected static void waitForSpan(final Matcher<SpanData> parentSpanMatcher)
{
- final Configuration conf =
MINI_CLUSTER_RULE.getTestingUtility().getConfiguration();
- Waiter.waitFor(conf, TimeUnit.SECONDS.toMillis(5), new MatcherPredicate<>(
- "Span for test failed to complete.", OTEL_CLASS_RULE::getSpans,
hasItem(parentSpanMatcher)));
+ UTIL.waitFor(TimeUnit.SECONDS.toMillis(5), new MatcherPredicate<>(
+ "Span for test failed to complete.", OTEL_EXT::getSpans,
hasItem(parentSpanMatcher)));
}
protected static Stream<SpanData> spanStream() {
- return OTEL_CLASS_RULE.getSpans().stream().filter(Objects::nonNull);
+ return OTEL_EXT.getSpans().stream().filter(Objects::nonNull);
}
- @Test
+ @TestTemplate
public void testScanAll() throws Exception {
List<Result> results = doScan(createScan(), -1);
// make sure all scanners are closed at RS side
-
MINI_CLUSTER_RULE.getTestingUtility().getHBaseCluster().getRegionServerThreads().stream()
+ UTIL.getHBaseCluster().getRegionServerThreads().stream()
.map(JVMClusterUtil.RegionServerThread::getRegionServer).forEach(
- rs -> assertEquals(
- "The scanner count of " + rs.getServerName() + " is "
- + rs.getRSRpcServices().getScannersCount(),
- 0, rs.getRSRpcServices().getScannersCount()));
+ rs -> assertEquals(0, rs.getRSRpcServices().getScannersCount(), "The
scanner count of "
+ + rs.getServerName() + " is " +
rs.getRSRpcServices().getScannersCount()));
Review Comment:
`rs.getRSRpcServices().getScannersCount()` is called twice per region server
(once as the assertion actual value and again to build the message). Store the
count in a local variable and reuse it so the assertion message always matches
the asserted value and avoids duplicate calls.
--
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]
