Close file InputStreams when copying segments#516
Conversation
ivanyu
force-pushed
the
ivanyu/fix-input-files-closing
branch
6 times, most recently
from
5dce06e to
ad75b20
Compare
jeqo
left a comment
jeqo
left a comment
There was a problem hiding this comment.
LGTM, nicely solved.
Thanks @funky-eyes for reporting, and @ivanyu for the fix!
|
Hi @jeqo when can this PR be merged? |
| new TransformFinisher(transformEnum, remoteLogSegmentMetadata.segmentSizeInBytes()); | ||
| uploadSegmentLog(remoteLogSegmentMetadata, transformFinisher, customMetadataBuilder); | ||
|
|
||
| final ChunkIndex chunkIndex = transformFinisher.chunkIndex(); |
There was a problem hiding this comment.
Should not it be so that the lifecycle of transformFinisher should be same as logSementInputStream? Saving chunkIndex into filed in rather a side effect, it is safe to get it here but the main purpose of the finisher is based on the underlying stream that is already closed afaiu. So I'd rather would not expose finisher from the try related to underlying stream or maybe would even try to make it auto closeable(not sure if it's possible) and close simultaneously with the underlying stream.
There was a problem hiding this comment.
It's safe because chunkIndex() is designed be called after the processing (otherwise IllegalStateException will be thrown). However, I moved TransformFinisher inside the try, have a look at the fixup.
There was a problem hiding this comment.
In general LGTM but not sure if we need a nested try, imo it could be just single one.
ivanyu
force-pushed
the
ivanyu/fix-input-files-closing
branch
from
d5c3767 to
8a86fee
Compare
4 participants
Closes #513
It's easier to review
RemoteStorageManager.javaif you hide white spaces in Github or your IDE.