Copilot commented on code in PR #2231:
URL: https://github.com/apache/phoenix/pull/2231#discussion_r2214555307
##########
phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileWriterTest.java:
##########
@@ -137,6 +137,16 @@ public void testLogFileReaderEmptyFile() throws
IOException {
reader.close();
}
+ @Test
+ public void testHeaderWrittenImmediately() throws IOException {
+ // This should write header immediately
+ initLogFileWriter();
+ // Verify file exists and has content (header should be written)
+ assertTrue("File should exist after init", localFs.exists(filePath));
+ assertEquals("File should have header written",
LogFileHeader.HEADERSIZE,writer.getLength());
Review Comment:
Missing space before 'writer.getLength()'. Should be
'LogFileHeader.HEADERSIZE, writer.getLength());'
```suggestion
assertEquals("File should have header written",
LogFileHeader.HEADERSIZE, writer.getLength());
```
##########
phoenix-core-server/src/main/java/org/apache/phoenix/replication/log/LogFileHeader.java:
##########
@@ -36,7 +34,7 @@ public class LogFileHeader implements LogFile.Header {
/** Current minor version of the replication log format */
static final int VERSION_MINOR = 0;
Review Comment:
The comment mentions '3 * Bytes.SIZEOF_BYTE' was changed to '2 *
Bytes.SIZEOF_BYTE' but there's no explanation for this change. This could
indicate a breaking change in the header format that should be documented.
```suggestion
// The header consists of the MAGIC array and two bytes for the major
and minor versions.
// Previously, this was calculated as MAGIC.length + 3 *
Bytes.SIZEOF_BYTE, but the format
// was updated to only include two version bytes. Ensure this matches
the serialized format.
```
##########
phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileFormatTest.java:
##########
@@ -406,6 +411,68 @@ public void testLogFileCorruptionPartialTrailer() throws
IOException {
assertEquals("Records read count mismatch", totalRecords,
readerContext.getRecordsRead());
}
+ @Test(expected=InvalidLogHeaderException.class)
+ public void testFailIfMissingHeader() throws IOException {
+ // Zero length file
+ byte[] data = new byte[0];
+ LogFileTestUtil.SeekableByteArrayInputStream input =
+ new LogFileTestUtil.SeekableByteArrayInputStream(data);
+ readerContext.setFileSize(data.length);
+ readerContext.setValidateTrailer(false);
+ reader.init(readerContext, input);
+ fail("Expected InvalidLogHeaderException for zero length file");
+ }
+
+ @Test(expected=InvalidLogHeaderException.class)
Review Comment:
[nitpick] Consider using assertThrows() instead of the deprecated
@Test(expected=...) pattern for better error message validation and more
precise exception testing.
```suggestion
assertThrows(InvalidLogHeaderException.class, () -> {
reader.init(readerContext, input);
});
}
@Test
```
##########
phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileFormatTest.java:
##########
@@ -406,6 +411,68 @@ public void testLogFileCorruptionPartialTrailer() throws
IOException {
assertEquals("Records read count mismatch", totalRecords,
readerContext.getRecordsRead());
}
+ @Test(expected=InvalidLogHeaderException.class)
Review Comment:
[nitpick] Consider using assertThrows() instead of the deprecated
@Test(expected=...) pattern for better error message validation and more
precise exception testing.
##########
phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileFormatTest.java:
##########
@@ -406,6 +411,68 @@ public void testLogFileCorruptionPartialTrailer() throws
IOException {
assertEquals("Records read count mismatch", totalRecords,
readerContext.getRecordsRead());
}
+ @Test(expected=InvalidLogHeaderException.class)
+ public void testFailIfMissingHeader() throws IOException {
+ // Zero length file
+ byte[] data = new byte[0];
+ LogFileTestUtil.SeekableByteArrayInputStream input =
+ new LogFileTestUtil.SeekableByteArrayInputStream(data);
+ readerContext.setFileSize(data.length);
+ readerContext.setValidateTrailer(false);
+ reader.init(readerContext, input);
+ fail("Expected InvalidLogHeaderException for zero length file");
+ }
+
+ @Test(expected=InvalidLogHeaderException.class)
+ public void testFailIfInvalidHeader() throws IOException {
+ initLogFileWriter();
+ writer.close(); // Writes valid trailer
+ byte[] data = writerBaos.toByteArray();
+ LogFileTestUtil.SeekableByteArrayInputStream input =
+ new LogFileTestUtil.SeekableByteArrayInputStream(data);
+ readerContext.setFileSize(data.length);
+ readerContext.setValidateTrailer(true);
+ data[0] = (byte) 'X'; // Corrupt the first magic byte
+ reader.init(readerContext, input);
+ fail("Expected InvalidLogHeaderException for file with corrupted
header magic");
+ }
+
+ @Test(expected=InvalidLogTrailerException.class)
+ public void testFailIfMissingTrailer() throws IOException {
+ initLogFileWriter();
+ writeBlock(writer, "B1", 0, 5);
+ // Don't close the writer, simulate missing trailer
+ byte[] data = writerBaos.toByteArray();
+ // Re-initialize reader with truncated data and trailer validation
enabled
+ LogFileTestUtil.SeekableByteArrayInputStream input =
+ new LogFileTestUtil.SeekableByteArrayInputStream(data);
+ readerContext.setFileSize(data.length);
+ // Enable trailer validation
+ readerContext.setValidateTrailer(true);
+ reader.init(readerContext, input);
+ fail("Expected InvalidLogTrailerException when trailer is missing");
+ }
+
+ @Test(expected=InvalidLogTrailerException.class)
Review Comment:
[nitpick] Consider using assertThrows() instead of the deprecated
@Test(expected=...) pattern for better error message validation and more
precise exception testing.
```suggestion
assertThrows(InvalidLogTrailerException.class, () ->
reader.init(readerContext, input));
}
@Test
```
##########
phoenix-core/src/test/java/org/apache/phoenix/replication/log/LogFileFormatTest.java:
##########
@@ -406,6 +411,68 @@ public void testLogFileCorruptionPartialTrailer() throws
IOException {
assertEquals("Records read count mismatch", totalRecords,
readerContext.getRecordsRead());
}
+ @Test(expected=InvalidLogHeaderException.class)
+ public void testFailIfMissingHeader() throws IOException {
+ // Zero length file
+ byte[] data = new byte[0];
+ LogFileTestUtil.SeekableByteArrayInputStream input =
+ new LogFileTestUtil.SeekableByteArrayInputStream(data);
+ readerContext.setFileSize(data.length);
+ readerContext.setValidateTrailer(false);
+ reader.init(readerContext, input);
+ fail("Expected InvalidLogHeaderException for zero length file");
+ }
+
+ @Test(expected=InvalidLogHeaderException.class)
+ public void testFailIfInvalidHeader() throws IOException {
+ initLogFileWriter();
+ writer.close(); // Writes valid trailer
+ byte[] data = writerBaos.toByteArray();
+ LogFileTestUtil.SeekableByteArrayInputStream input =
+ new LogFileTestUtil.SeekableByteArrayInputStream(data);
+ readerContext.setFileSize(data.length);
+ readerContext.setValidateTrailer(true);
+ data[0] = (byte) 'X'; // Corrupt the first magic byte
+ reader.init(readerContext, input);
+ fail("Expected InvalidLogHeaderException for file with corrupted
header magic");
+ }
+
+ @Test(expected=InvalidLogTrailerException.class)
Review Comment:
[nitpick] Consider using assertThrows() instead of the deprecated
@Test(expected=...) pattern for better error message validation and more
precise exception testing.
--
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: issues-unsubscr...@phoenix.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
