Copilot commented on code in PR #8292:
URL: https://github.com/apache/hbase/pull/8292#discussion_r3329098823
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompaction.java:
##########
@@ -743,7 +737,7 @@ public List<Path> compact(ThroughputController
throughputController, User user)
this.wait();
}
} catch (InterruptedException e) {
- Assume.assumeNoException(e);
+ Assumptions.abort(e.getMessage());
}
Review Comment:
When catching InterruptedException, the interrupt status is cleared.
Aborting the test without restoring the interrupt can cause subtle issues for
the test framework and any calling code. Restore the interrupt flag before
aborting, and use a non-null message instead of `e.getMessage()` which may be
null.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingToCellFlatMapMemStore.java:
##########
@@ -17,70 +17,70 @@
*/
package org.apache.hadoop.hbase.regionserver;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
+import java.util.stream.Stream;
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.hbase.Cell;
import org.apache.hadoop.hbase.CellComparatorImpl;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseConfiguration;
import org.apache.hadoop.hbase.KeyValue;
import org.apache.hadoop.hbase.MemoryCompactionPolicy;
-import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.ClassSize;
import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
import org.apache.hadoop.hbase.util.Threads;
-import org.junit.ClassRule;
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedClass;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
* compacted memstore test case
*/
-@Category({ RegionServerTests.class, LargeTests.class })
-@RunWith(Parameterized.class)
+@Tag(RegionServerTests.TAG)
+@Tag(MediumTests.TAG)
+@ParameterizedClass(name = "{index}: type={0}")
+@MethodSource("parameters")
public class TestCompactingToCellFlatMapMemStore extends
TestCompactingMemStore {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestCompactingToCellFlatMapMemStore.class);
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TestCompactingToCellFlatMapMemStore.class);
- @Parameterized.Parameters
- public static Object[] data() {
- return new Object[] { "CHUNK_MAP", "ARRAY_MAP" }; // test different
immutable indexes
+ public static Stream<Arguments> parameters() {
+ // test different immutable indexes
+ return Stream.of(Arguments.of("CHUNK_MAP"), Arguments.of("ARRAY_MAP"));
}
- private static final Logger LOG =
- LoggerFactory.getLogger(TestCompactingToCellFlatMapMemStore.class);
public final boolean toCellChunkMap;
+ private final String type;
Configuration conf;
//////////////////////////////////////////////////////////////////////////////
// Helpers
//////////////////////////////////////////////////////////////////////////////
Review Comment:
The `Configuration conf` field here is only used inside `createMemStore()`
and ends up shadowing the `conf` field in the parent class (even though the
parent field is private). This makes it very easy to confuse which
configuration is being used for region/store vs memstore setup. Prefer using a
local variable in `createMemStore()` and drop the field entirely.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCompactingMemStore.java:
##########
@@ -379,13 +374,13 @@ private long runSnapshot(final AbstractMemStore hmc,
boolean useForce) throws IO
MemStoreSnapshot snapshot = hmc.snapshot();
if (useForce) {
// Make some assertions about what just happened.
- assertTrue("History size has not increased", oldHistorySize <
snapshot.getDataSize());
+ assertTrue(oldHistorySize < snapshot.getDataSize(), "History size has
not increased");
long t = hmc.timeOfOldestEdit();
- assertTrue("Time of oldest edit is not Long.MAX_VALUE", t ==
Long.MAX_VALUE);
+ assertEquals(Long.MAX_VALUE, t, "Time of oldest edit is not
Long.MAX_VALUE");
hmc.clearSnapshot(snapshot.getId());
} else {
long t = hmc.timeOfOldestEdit();
- assertTrue("Time of oldest edit didn't remain the same", t ==
prevTimeStamp);
+ assertEquals(t, prevTimeStamp, "Time of oldest edit didn't remain the
same");
}
Review Comment:
`assertEquals` argument order is reversed here (`assertEquals(t,
prevTimeStamp, ...)`). While equality is symmetric, the expected/actual order
affects failure output and makes debugging harder. Use `prevTimeStamp` as
expected and `t` as actual.
--
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]
