MINOR; Remove cast for Records' slice #19661
Conversation
|
@jsancio there is a discussion before - see #19581 (comment) |
Thanks. I looked at that discussion. It is always safe for an implementation to override the return type with a subtype. This is because return types are covariant and Java supports it. |
| try { | ||
| return new FileRecords(file, channel, startPosition, startPosition + availableBytes, true); | ||
| } catch (IOException e) { | ||
| throw new UncheckedIOException(e); |
There was a problem hiding this comment.
In RecordBatchIterator, we convert an IOException to a KafkaException. Should we be consistent here?
There was a problem hiding this comment.
It seems the IOException is meaningless when the isSlice=true. Maybe we can have a new constructor of FileRecords which does not have IOException in the signature?
There was a problem hiding this comment.
In RecordBatchIterator, we convert an IOException to a KafkaException. Should we be consistent here?
Are you suggesting I change it to throw an UncheckedIOException? If so, that is what I did. I looked at the method declaration and we don't seem to document that KafkaException is thrown. It should be okay to change the type. What do you think?
There was a problem hiding this comment.
It seems the IOException is meaningless when the isSlice=true. Maybe we can have a new constructor of FileRecords which does not have IOException in the signature?
Good catch. Fixed it. Let me know what you think of my solution.
| @@ -1170,7 +1169,7 @@ public void testSliceEmptyRecords() { | |||
| */ | |||
| @ParameterizedTest | |||
| @ArgumentsSource(MemoryRecordsArgumentsProvider.class) | |||
| public void testSliceForAlreadySlicedMemoryRecords(Args args) throws IOException { | |||
| public void testSliceForAlreadySlicedMemoryRecords(Args args) { | |||
There was a problem hiding this comment.
Can we also remove explicit casting in this class i.e. at line 1092, 1093, etc. The similar we removed in FileRecordsTest.
There was a problem hiding this comment.
Thanks! I searched the entire repo for casts to MemoryRecord and FileRecords and I think we got all of them.
| @@ -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); | |||
There was a problem hiding this comment.
Do we still need this change? There are quite a few other places where we convert to KafkaException.
| FileRecords( | ||
| File file, | ||
| FileChannel channel, | ||
| int start, |
There was a problem hiding this comment.
Could we remove start since it's always 0? Also, we should direct open() to use this constructor, right?
There was a problem hiding this comment.
Yeah. Good catch. Should make the constructor much cleaner.
| FileChannel channel, | ||
| int start, | ||
| int end, | ||
| boolean isSlice |
There was a problem hiding this comment.
Could we just remove isSlice and add a comment that this is intended for slicing only?
There was a problem hiding this comment.
Yes. I removed this since open doesn't specify the start.
| // set the file position to the last byte in the file | ||
| channel.position(limit); | ||
| } | ||
| // if this is not a slice, update the file pointer to the end of the file |
There was a problem hiding this comment.
Should we change this comment since this constructor is not for slicing?
| this.start = start; | ||
| this.end = end; | ||
| this.isSlice = true; | ||
| this.size = new AtomicInteger(); |
There was a problem hiding this comment.
// don't check the file size if this is just a slice view
this.size = new AtomicInteger(end - start);
In Java return types are covariant. This means that method override can
override the return type with a subclass.