Copilot commented on code in PR #8206:
URL: https://github.com/apache/hbase/pull/8206#discussion_r3208433425
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncMetaRegionLocator.java:
##########
@@ -28,218 +28,155 @@
import static org.hamcrest.Matchers.hasItem;
import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension;
import io.opentelemetry.sdk.trace.data.SpanData;
+import java.io.IOException;
import java.util.List;
import java.util.concurrent.TimeUnit;
+import java.util.stream.Stream;
import org.apache.hadoop.conf.Configuration;
-import org.apache.hadoop.hbase.ConnectionRule;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
-import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.HBaseTestingUtil;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.HRegionLocation;
import org.apache.hadoop.hbase.MatcherPredicate;
-import org.apache.hadoop.hbase.MiniClusterRule;
import org.apache.hadoop.hbase.RegionLocations;
-import org.apache.hadoop.hbase.StartTestingClusterOption;
import org.apache.hadoop.hbase.TableName;
-import org.apache.hadoop.hbase.Waiter;
import org.apache.hadoop.hbase.client.RegionReplicaTestHelper.Locator;
import org.apache.hadoop.hbase.client.trace.StringTraceRenderer;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.testclassification.ClientTests;
import org.apache.hadoop.hbase.testclassification.MediumTests;
-import org.apache.hadoop.hbase.trace.OpenTelemetryClassRule;
-import org.apache.hadoop.hbase.trace.OpenTelemetryTestRule;
import org.apache.hadoop.hbase.trace.TraceUtil;
import org.hamcrest.Matcher;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
-import org.junit.experimental.runners.Enclosed;
-import org.junit.rules.ExternalResource;
-import org.junit.rules.RuleChain;
-import org.junit.rules.TestName;
-import org.junit.rules.TestRule;
-import org.junit.runner.RunWith;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+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;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Enclosed.class)
+import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
+
+@Tag(MediumTests.TAG)
+@Tag(ClientTests.TAG)
+@HBaseParameterizedTestTemplate(name = "[{index}]: registry = {0}")
public class TestAsyncMetaRegionLocator {
- private static final Logger logger =
LoggerFactory.getLogger(TestAsyncMetaRegionLocator.class);
- private static final class Setup extends ExternalResource {
+ private static final Logger LOG =
LoggerFactory.getLogger(TestAsyncMetaRegionLocator.class);
- private final MiniClusterRule miniClusterRule;
- private final ConnectionRule connectionRule;
+ private static final HBaseTestingUtil UTIL = new HBaseTestingUtil();
- private boolean initialized = false;
- private HBaseTestingUtil testUtil;
- private AsyncMetaRegionLocator locator;
- private ConnectionRegistry registry;
+ @RegisterExtension
+ private static final OpenTelemetryExtension OTEL_EXT =
OpenTelemetryExtension.create();
- public Setup(final ConnectionRule connectionRule, final MiniClusterRule
miniClusterRule) {
- this.connectionRule = connectionRule;
- this.miniClusterRule = miniClusterRule;
- }
+ private Class<? extends ConnectionRegistry> registryClass;
- public HBaseTestingUtil getTestingUtility() {
- assertInitialized();
- return testUtil;
- }
+ private ConnectionRegistry registry;
- public AsyncMetaRegionLocator getLocator() {
- assertInitialized();
- return locator;
- }
+ private AsyncMetaRegionLocator locator;
- private void assertInitialized() {
- if (!initialized) {
- throw new IllegalStateException("before method has not been called.");
- }
- }
+ public TestAsyncMetaRegionLocator(Class<? extends ConnectionRegistry>
registryClass) {
+ this.registryClass = registryClass;
+ }
- @Override
- protected void before() throws Throwable {
- final AsyncAdmin admin = connectionRule.getAsyncConnection().getAdmin();
- testUtil = miniClusterRule.getTestingUtility();
- HBaseTestingUtil.setReplicas(admin, TableName.META_TABLE_NAME, 3);
- testUtil.waitUntilNoRegionsInTransition();
- registry = ConnectionRegistryFactory.create(testUtil.getConfiguration(),
User.getCurrent());
- RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(testUtil,
registry);
- admin.balancerSwitch(false).get();
- locator = new AsyncMetaRegionLocator(registry);
- initialized = true;
- }
+ @SuppressWarnings("deprecation")
+ public static Stream<Arguments> parameters() {
+ return Stream.of(Arguments.of(RpcConnectionRegistry.class),
Arguments.of(MasterRegistry.class),
+ Arguments.of(ZKConnectionRegistry.class));
+ }
- @Override
- protected void after() {
- registry.close();
+ @BeforeAll
+ public static void setUpBeforeAll() throws Exception {
+ UTIL.startMiniCluster(3);
+ HBaseTestingUtil.setReplicas(UTIL.getAdmin(), TableName.META_TABLE_NAME,
3);
+ UTIL.waitUntilNoRegionsInTransition();
+ try (ConnectionRegistry registry =
+ ConnectionRegistryFactory.create(UTIL.getConfiguration(),
User.getCurrent())) {
+ RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(UTIL, registry);
}
+ UTIL.getAdmin().balancerSwitch(false, true);
}
- public static abstract class AbstractBase {
- private final OpenTelemetryClassRule otelClassRule =
OpenTelemetryClassRule.create();
- private final MiniClusterRule miniClusterRule;
- private final Setup setup;
-
- protected Matcher<SpanData> parentSpanMatcher;
- protected List<SpanData> spans;
- protected Matcher<SpanData> registryGetMetaRegionLocationsMatcher;
-
- @Rule
- public final TestRule classRule;
-
- @Rule
- public final OpenTelemetryTestRule otelTestRule = new
OpenTelemetryTestRule(otelClassRule);
-
- @Rule
- public TestName testName = new TestName();
-
- public AbstractBase() {
- miniClusterRule = MiniClusterRule.newBuilder()
-
.setMiniClusterOption(StartTestingClusterOption.builder().numWorkers(3).build())
- .setConfiguration(() -> {
- final Configuration conf = HBaseConfiguration.create();
- conf.setClass(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
- getConnectionRegistryClass(), ConnectionRegistry.class);
- return conf;
- }).build();
- final ConnectionRule connectionRule =
-
ConnectionRule.createAsyncConnectionRule(miniClusterRule::createAsyncConnection);
- setup = new Setup(connectionRule, miniClusterRule);
- classRule =
RuleChain.outerRule(otelClassRule).around(miniClusterRule).around(connectionRule)
- .around(setup);
- }
+ @AfterAll
+ public static void tearDownAfterAll() throws IOException {
+ UTIL.shutdownMiniCluster();
+ }
- protected abstract Class<? extends ConnectionRegistry>
getConnectionRegistryClass();
-
- @Test
- public void test() throws Exception {
- final AsyncMetaRegionLocator locator = setup.getLocator();
- final HBaseTestingUtil testUtil = setup.getTestingUtility();
-
- TraceUtil.trace(() -> {
- try {
- testLocator(miniClusterRule.getTestingUtility(),
TableName.META_TABLE_NAME,
- new Locator() {
- @Override
- public void updateCachedLocationOnError(HRegionLocation loc,
Throwable error) {
- locator.updateCachedLocationOnError(loc, error);
- }
-
- @Override
- public RegionLocations getRegionLocations(TableName tableName,
int replicaId,
- boolean reload) throws Exception {
- return locator.getRegionLocations(replicaId, reload).get();
- }
- });
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
- }, testName.getMethodName());
-
- final Configuration conf = testUtil.getConfiguration();
- parentSpanMatcher = allOf(hasName(testName.getMethodName()), hasEnded());
- Waiter.waitFor(conf, TimeUnit.SECONDS.toMillis(5),
- new MatcherPredicate<>(otelClassRule::getSpans,
hasItem(parentSpanMatcher)));
- spans = otelClassRule.getSpans();
- if (logger.isDebugEnabled()) {
- StringTraceRenderer renderer = new StringTraceRenderer(spans);
- renderer.render(logger::debug);
- }
- assertThat(spans, hasItem(parentSpanMatcher));
- final SpanData parentSpan =
spans.stream().filter(parentSpanMatcher::matches).findAny()
- .orElseThrow(AssertionError::new);
-
- registryGetMetaRegionLocationsMatcher =
- allOf(hasName(endsWith("ConnectionRegistry.getMetaRegionLocations")),
- hasParentSpanId(parentSpan), hasKind(SpanKind.INTERNAL), hasEnded());
- assertThat(spans, hasItem(registryGetMetaRegionLocationsMatcher));
- }
+ @BeforeEach
+ public void setUp() throws IOException {
+ Configuration conf = new Configuration(UTIL.getConfiguration());
+ conf.setClass(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
registryClass,
+ ConnectionRegistry.class);
+ registry = ConnectionRegistryFactory.create(conf, User.getCurrent());
+ locator = new AsyncMetaRegionLocator(registry);
}
- /**
- * Test covers when client is configured with {@link ZKConnectionRegistry}.
- */
- @Category({ MediumTests.class, ClientTests.class })
- public static class TestZKConnectionRegistry extends AbstractBase {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestZKConnectionRegistry.class);
-
- @Override
- protected Class<? extends ConnectionRegistry> getConnectionRegistryClass()
{
- return ZKConnectionRegistry.class;
- }
+ @AfterEach
+ public void tearDown() throws IOException {
+ Closeables.close(registry, true);
}
- /**
- * Test covers when client is configured with {@link RpcConnectionRegistry}.
- */
- @Category({ MediumTests.class, ClientTests.class })
- public static class TestRpcConnectionRegistry extends AbstractBase {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestRpcConnectionRegistry.class);
-
- @Override
- protected Class<? extends ConnectionRegistry> getConnectionRegistryClass()
{
- return RpcConnectionRegistry.class;
- }
+ @TestTemplate
+ public void test(TestInfo testInfo) {
+ String methodName = testInfo.getTestMethod().get().getName();
Review Comment:
`methodName` is derived from `testInfo.getTestMethod().get().getName()`,
which will be identical ("test") for all `@TestTemplate` invocations. Use
`testInfo.getDisplayName()` (or otherwise include the current
`registryClass`/index) when naming the trace + matchers so each parameterized
invocation asserts against its own spans.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncMetaRegionLocator.java:
##########
@@ -28,218 +28,155 @@
import static org.hamcrest.Matchers.hasItem;
import io.opentelemetry.api.trace.SpanKind;
+import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension;
import io.opentelemetry.sdk.trace.data.SpanData;
+import java.io.IOException;
import java.util.List;
import java.util.concurrent.TimeUnit;
+import java.util.stream.Stream;
import org.apache.hadoop.conf.Configuration;
-import org.apache.hadoop.hbase.ConnectionRule;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
-import org.apache.hadoop.hbase.HBaseConfiguration;
+import org.apache.hadoop.hbase.HBaseParameterizedTestTemplate;
import org.apache.hadoop.hbase.HBaseTestingUtil;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.HRegionLocation;
import org.apache.hadoop.hbase.MatcherPredicate;
-import org.apache.hadoop.hbase.MiniClusterRule;
import org.apache.hadoop.hbase.RegionLocations;
-import org.apache.hadoop.hbase.StartTestingClusterOption;
import org.apache.hadoop.hbase.TableName;
-import org.apache.hadoop.hbase.Waiter;
import org.apache.hadoop.hbase.client.RegionReplicaTestHelper.Locator;
import org.apache.hadoop.hbase.client.trace.StringTraceRenderer;
import org.apache.hadoop.hbase.security.User;
import org.apache.hadoop.hbase.testclassification.ClientTests;
import org.apache.hadoop.hbase.testclassification.MediumTests;
-import org.apache.hadoop.hbase.trace.OpenTelemetryClassRule;
-import org.apache.hadoop.hbase.trace.OpenTelemetryTestRule;
import org.apache.hadoop.hbase.trace.TraceUtil;
import org.hamcrest.Matcher;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
-import org.junit.experimental.runners.Enclosed;
-import org.junit.rules.ExternalResource;
-import org.junit.rules.RuleChain;
-import org.junit.rules.TestName;
-import org.junit.rules.TestRule;
-import org.junit.runner.RunWith;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+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;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@RunWith(Enclosed.class)
+import org.apache.hbase.thirdparty.com.google.common.io.Closeables;
+
+@Tag(MediumTests.TAG)
+@Tag(ClientTests.TAG)
+@HBaseParameterizedTestTemplate(name = "[{index}]: registry = {0}")
public class TestAsyncMetaRegionLocator {
- private static final Logger logger =
LoggerFactory.getLogger(TestAsyncMetaRegionLocator.class);
- private static final class Setup extends ExternalResource {
+ private static final Logger LOG =
LoggerFactory.getLogger(TestAsyncMetaRegionLocator.class);
- private final MiniClusterRule miniClusterRule;
- private final ConnectionRule connectionRule;
+ private static final HBaseTestingUtil UTIL = new HBaseTestingUtil();
- private boolean initialized = false;
- private HBaseTestingUtil testUtil;
- private AsyncMetaRegionLocator locator;
- private ConnectionRegistry registry;
+ @RegisterExtension
+ private static final OpenTelemetryExtension OTEL_EXT =
OpenTelemetryExtension.create();
- public Setup(final ConnectionRule connectionRule, final MiniClusterRule
miniClusterRule) {
- this.connectionRule = connectionRule;
- this.miniClusterRule = miniClusterRule;
- }
+ private Class<? extends ConnectionRegistry> registryClass;
- public HBaseTestingUtil getTestingUtility() {
- assertInitialized();
- return testUtil;
- }
+ private ConnectionRegistry registry;
- public AsyncMetaRegionLocator getLocator() {
- assertInitialized();
- return locator;
- }
+ private AsyncMetaRegionLocator locator;
- private void assertInitialized() {
- if (!initialized) {
- throw new IllegalStateException("before method has not been called.");
- }
- }
+ public TestAsyncMetaRegionLocator(Class<? extends ConnectionRegistry>
registryClass) {
+ this.registryClass = registryClass;
+ }
- @Override
- protected void before() throws Throwable {
- final AsyncAdmin admin = connectionRule.getAsyncConnection().getAdmin();
- testUtil = miniClusterRule.getTestingUtility();
- HBaseTestingUtil.setReplicas(admin, TableName.META_TABLE_NAME, 3);
- testUtil.waitUntilNoRegionsInTransition();
- registry = ConnectionRegistryFactory.create(testUtil.getConfiguration(),
User.getCurrent());
- RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(testUtil,
registry);
- admin.balancerSwitch(false).get();
- locator = new AsyncMetaRegionLocator(registry);
- initialized = true;
- }
+ @SuppressWarnings("deprecation")
+ public static Stream<Arguments> parameters() {
+ return Stream.of(Arguments.of(RpcConnectionRegistry.class),
Arguments.of(MasterRegistry.class),
+ Arguments.of(ZKConnectionRegistry.class));
+ }
- @Override
- protected void after() {
- registry.close();
+ @BeforeAll
+ public static void setUpBeforeAll() throws Exception {
+ UTIL.startMiniCluster(3);
+ HBaseTestingUtil.setReplicas(UTIL.getAdmin(), TableName.META_TABLE_NAME,
3);
+ UTIL.waitUntilNoRegionsInTransition();
+ try (ConnectionRegistry registry =
+ ConnectionRegistryFactory.create(UTIL.getConfiguration(),
User.getCurrent())) {
+ RegionReplicaTestHelper.waitUntilAllMetaReplicasAreReady(UTIL, registry);
}
+ UTIL.getAdmin().balancerSwitch(false, true);
}
- public static abstract class AbstractBase {
- private final OpenTelemetryClassRule otelClassRule =
OpenTelemetryClassRule.create();
- private final MiniClusterRule miniClusterRule;
- private final Setup setup;
-
- protected Matcher<SpanData> parentSpanMatcher;
- protected List<SpanData> spans;
- protected Matcher<SpanData> registryGetMetaRegionLocationsMatcher;
-
- @Rule
- public final TestRule classRule;
-
- @Rule
- public final OpenTelemetryTestRule otelTestRule = new
OpenTelemetryTestRule(otelClassRule);
-
- @Rule
- public TestName testName = new TestName();
-
- public AbstractBase() {
- miniClusterRule = MiniClusterRule.newBuilder()
-
.setMiniClusterOption(StartTestingClusterOption.builder().numWorkers(3).build())
- .setConfiguration(() -> {
- final Configuration conf = HBaseConfiguration.create();
- conf.setClass(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
- getConnectionRegistryClass(), ConnectionRegistry.class);
- return conf;
- }).build();
- final ConnectionRule connectionRule =
-
ConnectionRule.createAsyncConnectionRule(miniClusterRule::createAsyncConnection);
- setup = new Setup(connectionRule, miniClusterRule);
- classRule =
RuleChain.outerRule(otelClassRule).around(miniClusterRule).around(connectionRule)
- .around(setup);
- }
+ @AfterAll
+ public static void tearDownAfterAll() throws IOException {
+ UTIL.shutdownMiniCluster();
+ }
- protected abstract Class<? extends ConnectionRegistry>
getConnectionRegistryClass();
-
- @Test
- public void test() throws Exception {
- final AsyncMetaRegionLocator locator = setup.getLocator();
- final HBaseTestingUtil testUtil = setup.getTestingUtility();
-
- TraceUtil.trace(() -> {
- try {
- testLocator(miniClusterRule.getTestingUtility(),
TableName.META_TABLE_NAME,
- new Locator() {
- @Override
- public void updateCachedLocationOnError(HRegionLocation loc,
Throwable error) {
- locator.updateCachedLocationOnError(loc, error);
- }
-
- @Override
- public RegionLocations getRegionLocations(TableName tableName,
int replicaId,
- boolean reload) throws Exception {
- return locator.getRegionLocations(replicaId, reload).get();
- }
- });
- } catch (Exception e) {
- throw new RuntimeException(e);
- }
- }, testName.getMethodName());
-
- final Configuration conf = testUtil.getConfiguration();
- parentSpanMatcher = allOf(hasName(testName.getMethodName()), hasEnded());
- Waiter.waitFor(conf, TimeUnit.SECONDS.toMillis(5),
- new MatcherPredicate<>(otelClassRule::getSpans,
hasItem(parentSpanMatcher)));
- spans = otelClassRule.getSpans();
- if (logger.isDebugEnabled()) {
- StringTraceRenderer renderer = new StringTraceRenderer(spans);
- renderer.render(logger::debug);
- }
- assertThat(spans, hasItem(parentSpanMatcher));
- final SpanData parentSpan =
spans.stream().filter(parentSpanMatcher::matches).findAny()
- .orElseThrow(AssertionError::new);
-
- registryGetMetaRegionLocationsMatcher =
- allOf(hasName(endsWith("ConnectionRegistry.getMetaRegionLocations")),
- hasParentSpanId(parentSpan), hasKind(SpanKind.INTERNAL), hasEnded());
- assertThat(spans, hasItem(registryGetMetaRegionLocationsMatcher));
- }
+ @BeforeEach
+ public void setUp() throws IOException {
+ Configuration conf = new Configuration(UTIL.getConfiguration());
+ conf.setClass(HConstants.CLIENT_CONNECTION_REGISTRY_IMPL_CONF_KEY,
registryClass,
+ ConnectionRegistry.class);
+ registry = ConnectionRegistryFactory.create(conf, User.getCurrent());
Review Comment:
OpenTelemetry spans are not cleared between parameterized invocations, and
this test reuses the same span name ("test"). This can cause
`hasItem(parentSpanMatcher)` to match spans from a previous invocation, leading
to false positives/flaky behavior. Clear the extension's span reservoir in
`@BeforeEach` (e.g., `OTEL_EXT.clearSpans()`) and/or ensure the traced span
name is unique per invocation.
--
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]
