KAFKA-6499: Do not write offset checkpoint file with empty offset map #4492
Conversation
|
retest this please |
| @Test | ||
| public void testNotWriteEmptyMap() throws IOException { | ||
| // we do not need to worry about file name unqiueness since this file should not be created | ||
| File f = new File(TestUtils.tempDirectory().getAbsolutePath(), "kafka.tmp"); |
|
|
||
| assertEquals(Collections.<TopicPartition, Long>emptyMap(), checkpoint.read()); | ||
|
|
||
| checkpoint.delete(); |
There was a problem hiding this comment.
Why do we need a delete here if the file was never created?
There was a problem hiding this comment.
+1 - this line and the next shouldn't be needed
There was a problem hiding this comment.
I added this line intentionally to make sure delete() would not bark if the file does not exist, i.e. internally it calls deleteIfExists.
There was a problem hiding this comment.
fair enough - we still don't need the assertion though as it is already true as of line 69
There was a problem hiding this comment.
I'll remove the assertion.
| @@ -66,6 +66,10 @@ public OffsetCheckpoint(final File file) { | |||
| * @throws IOException if any file operation fails with an IO exception | |||
| */ | |||
| public void write(final Map<TopicPartition, Long> offsets) throws IOException { | |||
| // if there is no offsets, skip writing the file to save disk IOs | |||
| if (offsets.isEmpty()) | |||
There was a problem hiding this comment.
should we use java style here? i.e., if(..) {}?
| @@ -56,4 +57,20 @@ public void testReadWrite() throws IOException { | |||
| checkpoint.delete(); | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| public void testNotWriteEmptyMap() throws IOException { | |||
There was a problem hiding this comment.
shouldNotWriteCheckpointWhenNoOffsets?
|
|
||
| assertEquals(Collections.<TopicPartition, Long>emptyMap(), checkpoint.read()); | ||
|
|
||
| checkpoint.delete(); |
There was a problem hiding this comment.
+1 - this line and the next shouldn't be needed
…t-write-checkpoint-with-empty-offset
|
Refer to this link for build results (access rights to CI server needed): |
|
retest this please |
|
Refer to this link for build results (access rights to CI server needed): |
…t-write-checkpoint-with-empty-offset
|
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): |
…#4492) * In Checkpoint.write(), if the offset map passed in is empty, skip the writing of the file which would only contain version number and the empty size. From the reading pov, it is the same as no file existed. * Add related unit tests. * Minor fixes on log4j messages. Reviewers: Bill Bejeck <bill@confluent.io>, Damian Guy <damian@confluent.io>, Matthias J. Sax <matthias@confluent.io>
|
Cherry-picked to 1.1. Thanks for the reviews. |
guozhangwang
deleted the
K6499-not-write-checkpoint-with-empty-offset
branch
In Checkpoint.write(), if the offset map passed in is empty, skip the writing of the file which would only contain version number and the empty size. From the reading pov, it is the same as no file existed.
Add related unit tests.
Minor fixes on log4j messages.
Committer Checklist (excluded from commit message)