KAFKA-4549: Change to call flush method before writeEndMark method in close method of KafkaLZ4BlockOutputStream #2265
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
fossamagna
changed the title
fossamagna
changed the title
There was a problem hiding this comment.
@fossamagna : Thanks for finding this. Left a couple of comments.
| flush(); | ||
| writeEndMark(); |
There was a problem hiding this comment.
Should we call flush() again after writeEndMark()?
There was a problem hiding this comment.
Have a point. We should call flush in consideration of actually may be not flushed EndMark after writeEndMark().
I add call flush() after writeEndMark().
| @@ -64,7 +64,7 @@ public void testKafkaLZ4() throws IOException { | |||
| ByteArrayOutputStream output = new ByteArrayOutputStream(); | |||
| KafkaLZ4BlockOutputStream lz4 = new KafkaLZ4BlockOutputStream(output, this.useBrokenFlagDescriptorChecksum); | |||
| lz4.write(this.payload, 0, this.payload.length); | |||
| lz4.flush(); | |||
| lz4.close(); | |||
There was a problem hiding this comment.
Should we also add code to verify the end mark?
There was a problem hiding this comment.
I add code to verify the end mark.
There was a problem hiding this comment.
Also, it would be good to verify that flush still behaves as expected if called without close.
There was a problem hiding this comment.
I add test pattern without close.
It's consideration that EndMark bytes may not be flushed after writeEndMark().
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Thanks for the review @junrao. I add test code to verify the end mark. |
| @@ -64,7 +64,7 @@ public void testKafkaLZ4() throws IOException { | |||
| ByteArrayOutputStream output = new ByteArrayOutputStream(); | |||
| KafkaLZ4BlockOutputStream lz4 = new KafkaLZ4BlockOutputStream(output, this.useBrokenFlagDescriptorChecksum); | |||
| lz4.write(this.payload, 0, this.payload.length); | |||
| lz4.flush(); | |||
| lz4.close(); | |||
There was a problem hiding this comment.
Also, it would be good to verify that flush still behaves as expected if called without close.
| @@ -259,6 +259,7 @@ private void ensureNotFinished() { | |||
| @Override | |||
| public void close() throws IOException { | |||
| if (!finished) { | |||
| flush(); | |||
There was a problem hiding this comment.
Do we really need to call flush here? It's a bit odd to have to do it twice. Would writeBlock be enough?
There was a problem hiding this comment.
I agree. It is enough if writeBlock () is called.
|
@ijuma Thanks for the review. I have updated the PR. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Thanks for the PR, LGTM. Merged to trunk after fixing the assertions to have |
…tStream.close() Author: MURAKAMI Masahiko <fossamagna2@gmail.com> Reviewers: Jun Rao <junrao@gmail.com>, Ismael Juma <ismael@juma.me.uk> Closes apache#2265 from fossamagna/fix-lz4outputstream-close
No description provided.