Copilot commented on code in PR #8165:
URL: https://github.com/apache/hbase/pull/8165#discussion_r3162390822
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockHeaderCorruption.java:
##########
@@ -453,39 +468,25 @@ public Object readFully() throws IOException {
*/
public static class HFileBlockChannelPositionIterator implements Closeable {
- private final HFileTestRule hFileTestRule;
+ private final Path hfsPath;
private final HFile.Reader reader;
private final HFileBlock.BlockIterator iter;
private HFileBlockChannelPosition current = null;
- public HFileBlockChannelPositionIterator(HFileTestRule hFileTestRule)
throws IOException {
- Configuration conf = hFileTestRule.getConfiguration();
- HFileSystem hfs = hFileTestRule.getHFileSystem();
- Path hfsPath = hFileTestRule.getPath();
-
- HFile.Reader reader = null;
- HFileBlock.BlockIterator iter = null;
- try {
- reader = HFile.createReader(hfs, hfsPath, CacheConfig.DISABLED, true,
conf);
- HFileBlock.FSReader fsreader = reader.getUncachedBlockReader();
- // The read block offset cannot out of the range:0,loadOnOpenDataOffset
- iter = fsreader.blockRange(0,
reader.getTrailer().getLoadOnOpenDataOffset());
- } catch (IOException e) {
- if (reader != null) {
- closeQuietly(reader::close);
- }
- throw e;
- }
-
- this.hFileTestRule = hFileTestRule;
- this.reader = reader;
- this.iter = iter;
+ public HFileBlockChannelPositionIterator(HFileSystem hfs, Path hfsPath)
throws IOException {
+ this.hfsPath = hfsPath;
+ this.reader = HFile.createReader(hfs, hfsPath, CacheConfig.DISABLED,
true, hfs.getConf());
+ HFileBlock.FSReader fsreader = reader.getUncachedBlockReader();
+ // The read block offset cannot out of the range:0,loadOnOpenDataOffset
+ this.iter = fsreader.blockRange(0,
reader.getTrailer().getLoadOnOpenDataOffset());
}
public boolean hasNext() throws IOException {
HFileBlock next = iter.nextBlock();
- SeekableByteChannel channel = hFileTestRule.getRWChannel();
if (next != null) {
+ java.nio.file.Path p =
FileSystems.getDefault().getPath(hfsPath.toString());
+ SeekableByteChannel channel = Files.newByteChannel(p,
StandardOpenOption.READ,
+ StandardOpenOption.WRITE, StandardOpenOption.DSYNC);
current = new HFileBlockChannelPosition(channel, next.getOffset());
Review Comment:
`hasNext()` opens a new `SeekableByteChannel` for every block and stores it
in `current`, but the iterator only closes `current` in `close()`. Any
`HFileBlockChannelPosition` returned via `next()` will keep its channel open
unless the caller closes it, which is easy to miss and can leak file
descriptors across the test run. Consider making the iterator own a single RW
channel (open in the constructor, close in `close()`) and have
`HFileBlockChannelPosition` not own/close the channel, or alternatively close
the previous `current` before overwriting it and ensure all returned positions
are closed by the iteration logic.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockHeaderCorruption.java:
##########
@@ -453,39 +468,25 @@ public Object readFully() throws IOException {
*/
public static class HFileBlockChannelPositionIterator implements Closeable {
- private final HFileTestRule hFileTestRule;
+ private final Path hfsPath;
private final HFile.Reader reader;
private final HFileBlock.BlockIterator iter;
private HFileBlockChannelPosition current = null;
- public HFileBlockChannelPositionIterator(HFileTestRule hFileTestRule)
throws IOException {
- Configuration conf = hFileTestRule.getConfiguration();
- HFileSystem hfs = hFileTestRule.getHFileSystem();
- Path hfsPath = hFileTestRule.getPath();
-
- HFile.Reader reader = null;
- HFileBlock.BlockIterator iter = null;
- try {
- reader = HFile.createReader(hfs, hfsPath, CacheConfig.DISABLED, true,
conf);
- HFileBlock.FSReader fsreader = reader.getUncachedBlockReader();
- // The read block offset cannot out of the range:0,loadOnOpenDataOffset
- iter = fsreader.blockRange(0,
reader.getTrailer().getLoadOnOpenDataOffset());
- } catch (IOException e) {
- if (reader != null) {
- closeQuietly(reader::close);
- }
- throw e;
- }
-
- this.hFileTestRule = hFileTestRule;
- this.reader = reader;
- this.iter = iter;
+ public HFileBlockChannelPositionIterator(HFileSystem hfs, Path hfsPath)
throws IOException {
+ this.hfsPath = hfsPath;
+ this.reader = HFile.createReader(hfs, hfsPath, CacheConfig.DISABLED,
true, hfs.getConf());
+ HFileBlock.FSReader fsreader = reader.getUncachedBlockReader();
+ // The read block offset cannot out of the range:0,loadOnOpenDataOffset
+ this.iter = fsreader.blockRange(0,
reader.getTrailer().getLoadOnOpenDataOffset());
}
public boolean hasNext() throws IOException {
HFileBlock next = iter.nextBlock();
- SeekableByteChannel channel = hFileTestRule.getRWChannel();
if (next != null) {
+ java.nio.file.Path p =
FileSystems.getDefault().getPath(hfsPath.toString());
+ SeekableByteChannel channel = Files.newByteChannel(p,
StandardOpenOption.READ,
+ StandardOpenOption.WRITE, StandardOpenOption.DSYNC);
Review Comment:
`FileSystems.getDefault().getPath(hfsPath.toString())` assumes `hfsPath` is
a plain local path. If `hfsPath` includes a URI scheme (e.g. `file:/...`) or
the test filesystem is not local (e.g. `hdfs://...`), this will throw
`InvalidPathException` and the test cannot work anyway since it relies on NIO
read/write. Consider asserting/assuming a local test FS up front (skip
otherwise) and using `hfsPath.toUri()` to build the NIO path for the local case.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockHeaderCorruption.java:
##########
@@ -75,32 +71,52 @@
* HBase checksum validation can be applied. As of now, this is just
* {@code onDiskSizeWithoutHeader}.
*/
-@Category({ IOTests.class, SmallTests.class })
+@Tag(IOTests.TAG)
+@Tag(SmallTests.TAG)
public class TestHFileBlockHeaderCorruption {
private static final Logger LOG =
LoggerFactory.getLogger(TestHFileBlockHeaderCorruption.class);
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestHFileBlockHeaderCorruption.class);
+ private static final HBaseTestingUtil UTIL = new HBaseTestingUtil();
+ private static HFileSystem HFS;
+ private HFileContext hfileCtx;
+ private Path path;
- private final HFileTestRule hFileTestRule;
-
- @Rule
- public final RuleChain ruleChain;
+ @BeforeAll
+ public static void setUpBeforeAll() throws IOException {
+ HFS = (HFileSystem) HFileSystem.get(UTIL.getConfiguration());
+ }
- public TestHFileBlockHeaderCorruption() throws IOException {
- TestName testName = new TestName();
- hFileTestRule = new HFileTestRule(new HBaseTestingUtil(), testName);
- ruleChain = RuleChain.outerRule(testName).around(hFileTestRule);
+ @BeforeEach
+ public void setUp(TestInfo testInfo) throws IOException {
+ path = new Path(UTIL.getDataTestDirOnTestFS(),
testInfo.getTestMethod().get().getName());
+ hfileCtx = new HFileContextBuilder().withBlockSize(4 *
1024).withHBaseCheckSum(true).build();
Review Comment:
`testInfo.getTestMethod().get()` will throw a generic
`NoSuchElementException` if JUnit can't resolve a method (e.g., some
dynamic/templated executions). Prefer `orElseThrow(...)` with a clear message,
or use `testInfo.getDisplayName()` to derive the path name without assuming the
Optional is present.
--
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]
