junrao commented on code in PR #19661:
URL: https://github.com/apache/kafka/pull/19661#discussion_r2082362002
##########
clients/src/main/java/org/apache/kafka/common/record/FileRecords.java:
##########
@@ -54,34 +54,62 @@ public class FileRecords extends AbstractRecords implements
Closeable {
* The {@code FileRecords.open} methods should be used instead of this
constructor whenever possible.
* The constructor is visible for tests.
*/
- FileRecords(File file,
- FileChannel channel,
- int start,
- int end,
- boolean isSlice) throws IOException {
+ FileRecords(
+ File file,
+ FileChannel channel,
+ int start,
Review Comment:
Could we remove start since it's always 0? Also, we should direct `open()`
to use this constructor, right?
##########
clients/src/main/java/org/apache/kafka/common/record/RecordBatchIterator.java:
##########
@@ -41,7 +41,7 @@ protected T makeNext() {
} catch (EOFException e) {
throw new CorruptRecordException("Unexpected EOF while attempting
to read the next batch", e);
} catch (IOException e) {
- throw new KafkaException(e);
+ throw new UncheckedIOException(e);
Review Comment:
Do we still need this change? There are quite a few other places where we
convert to KafkaException.
##########
clients/src/main/java/org/apache/kafka/common/record/FileRecords.java:
##########
@@ -54,34 +54,62 @@ public class FileRecords extends AbstractRecords implements
Closeable {
* The {@code FileRecords.open} methods should be used instead of this
constructor whenever possible.
* The constructor is visible for tests.
*/
- FileRecords(File file,
- FileChannel channel,
- int start,
- int end,
- boolean isSlice) throws IOException {
+ FileRecords(
+ File file,
+ FileChannel channel,
+ int start,
+ int end
+ ) throws IOException {
this.file = file;
this.channel = channel;
this.start = start;
this.end = end;
- this.isSlice = isSlice;
+ this.isSlice = false;
this.size = new AtomicInteger();
- if (isSlice) {
- // don't check the file size if this is just a slice view
- size.set(end - start);
- } else {
- if (channel.size() > Integer.MAX_VALUE)
- throw new KafkaException("The size of segment " + file + " ("
+ channel.size() +
- ") is larger than the maximum allowed segment size of
" + Integer.MAX_VALUE);
+ if (channel.size() > Integer.MAX_VALUE) {
+ throw new KafkaException(
+ "The size of segment " + file + " (" + channel.size() +
+ ") is larger than the maximum allowed segment size of " +
Integer.MAX_VALUE
+ );
+ }
+
+ int limit = Math.min((int) channel.size(), end);
+ size.set(limit - start);
- int limit = Math.min((int) channel.size(), end);
- size.set(limit - start);
+ // if this is not a slice, update the file pointer to the end of the
file
+ // set the file position to the last byte in the file
+ channel.position(limit);
+
+ batches = batchesFrom(start);
+ }
- // if this is not a slice, update the file pointer to the end of
the file
- // set the file position to the last byte in the file
- channel.position(limit);
+ /**
+ * The {@code FileRecords.open} methods should be used instead of this
constructor whenever possible.
+ *
+ * isSlice must be true. This overloaded constructor avoids having to
declare a checked IO exception.
+ */
+ private FileRecords(
+ File file,
+ FileChannel channel,
+ int start,
+ int end,
+ boolean isSlice
Review Comment:
Could we just remove isSlice and add a comment that this is intended for
slicing only?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
