KAFKA-4902: Utils#delete should correctly handle I/O errors and symlinks #2691

cmccabe · 2017-03-15T15:40:11Z

No description provided.

asfbot · 2017-03-15T15:41:21Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2195/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-03-15T15:41:30Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2194/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-03-15T15:41:34Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2197/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-15T16:35:50Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2195/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-15T16:41:24Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2198/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-03-15T17:07:05Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2196/
Test FAILed (JDK 8 and Scala 2.12).

cmccabe · 2017-03-16T16:13:24Z

retest this please

asfbot · 2017-03-16T16:20:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2226/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-16T16:59:53Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2223/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-16T17:31:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2224/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-20T17:26:56Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2279/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-20T17:33:08Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2279/
Test PASSed (JDK 7 and Scala 2.10).

asfbot · 2017-03-20T17:37:40Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2282/
Test PASSed (JDK 8 and Scala 2.11).

junrao

@cmccabe : Thanks for the patch. Looks good to me. Just a couple of minor comments.

junrao · 2017-03-20T20:32:02Z

streams/src/main/java/org/apache/kafka/streams/state/internals/Segments.java

+                try {
+                    segment.destroy();
+                } catch (IOException e) {
+                    log.error("Error destroying segment id {}", segment.id, e);


Probably include both segment.id and and segment.name()?

Seems like it would be better to add a toString to the segment or superclass.

Good point. It looks like RocksDBStore already has a toString method. So I will just rely on that.

I did not find any toString in RocksDBStore so I added one in Segment (which can then expose the id, which is not something that RocksDBStore knows about).

junrao · 2017-03-20T20:32:14Z

clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java

+        TestUtils.tempDirectory(tempDir.toPath(), "b");
+        TestUtils.tempDirectory(tempDir2.toPath(), "c");
+        Utils.delete(tempDir); // deleting a directory hierarchy works
+        Utils.delete(tempDir); // deleting a non-existent directory hierarchy works


In this test, should we verify that the file/directory is actually gone after deletion?

I agree with jun, makes sense to test that the removal succeeded.

ijuma

Thanks for the PR, left a couple of minor comments. Great to use the better API to delete files recursively. I have been meaning to do that for a while. :)

ijuma · 2017-03-21T11:31:02Z

streams/src/main/java/org/apache/kafka/streams/state/internals/Segments.java

+                try {
+                    segment.destroy();
+                } catch (IOException e) {
+                    log.error("Error destroying segment id {}", segment.id, e);


Seems like it would be better to add a toString to the segment or superclass.

ijuma · 2017-03-21T11:32:36Z

streams/src/test/java/org/apache/kafka/streams/processor/internals/StateDirectoryTest.java

-            Utils.delete(stateDir);
-        }
+    public void cleanup() throws IOException {
+        Utils.delete(stateDir);


Have you verified that the exists was not there for a reason? Otherwise we may get failures during cleanup, which may obscure the real issue?

This exists is not necessary because Utils#delete does not fail when given a non-existent path. (It had this behavior earlier as well, this patch didn't change it.)

asfbot · 2017-03-27T22:20:25Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2429/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-03-27T22:26:46Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2425/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-27T22:33:32Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2425/
Test FAILed (JDK 7 and Scala 2.10).

cmccabe · 2017-03-28T17:44:00Z

retest this please

asfbot · 2017-03-28T17:45:42Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2464/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-03-28T17:46:15Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2468/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-28T17:46:25Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2464/
Test FAILed (JDK 7 and Scala 2.10).

asfbot · 2017-03-28T18:15:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2468/
Test FAILed (JDK 8 and Scala 2.12).

asfbot · 2017-03-28T18:15:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2472/
Test FAILed (JDK 8 and Scala 2.11).

asfbot · 2017-03-28T18:15:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2468/
Test FAILed (JDK 7 and Scala 2.10).

ijuma · 2017-03-29T15:02:08Z

Can you please rebase/merge from trunk so that the checkstyle issue that is preventing the PR builds from passing is fixed?

cmccabe · 2017-03-29T16:38:36Z

retest this please

asfbot · 2017-03-29T17:26:26Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2503/
Test PASSed (JDK 8 and Scala 2.12).

asfbot · 2017-03-29T17:30:51Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2507/
Test PASSed (JDK 8 and Scala 2.11).

asfbot · 2017-03-29T17:32:59Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2503/
Test FAILed (JDK 7 and Scala 2.10).

ijuma

LGTM, merging to trunk after a couple of minor tweaks: added Segment.toString and tweaked a few assertions.

ijuma · 2017-03-30T12:22:22Z

clients/src/main/java/org/apache/kafka/common/utils/Utils.java

@@ -568,19 +572,30 @@ public static ByteBuffer ensureCapacity(ByteBuffer existingBuffer, int newLength
     *
     * @param file The root file at which to begin deleting
     */
-    public static void delete(File file) {
-        if (file == null) {
+    public static void delete(final File file) throws IOException {


Maybe we should rename this to deleteIfExists since we don't throw an error if file doesn't exist. Probably in a subsequent PR if that makes sense to you.

ijuma · 2017-03-30T12:39:12Z

clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java

+
+        // Test that deleting a non-existent directory hierarchy works.
+        Utils.delete(tempDir);
+        assertFalse(Files.isDirectory(tempDir.toPath()));


I changed all the isDirectory checks to exists. Both work, but I think the latter is a bit more explicit about what we're checking.

cmccabe force-pushed the KAFKA-4902 branch from 8670331 to 6a8d186 Compare March 15, 2017 15:50

cmccabe force-pushed the KAFKA-4902 branch from 6a8d186 to 616e81b Compare March 16, 2017 16:14

cmccabe force-pushed the KAFKA-4902 branch from 616e81b to 37c1823 Compare March 20, 2017 16:36

junrao reviewed Mar 20, 2017

View reviewed changes

ijuma reviewed Mar 21, 2017

View reviewed changes

cmccabe force-pushed the KAFKA-4902 branch from 37c1823 to 63248a8 Compare March 27, 2017 21:13

cmccabe force-pushed the KAFKA-4902 branch from 63248a8 to 33f8d56 Compare March 28, 2017 18:13

cmccabe added 3 commits March 29, 2017 09:39

KAFKA-4902: Utils#delete should correctly handle I/O errors and symlinks

2f189b4

UtilsTest: test that we have actually deleted the files in question

7606535

Segments.java: rely on RocksDBStore#toString

2b24838

cmccabe force-pushed the KAFKA-4902 branch from 33f8d56 to 2b24838 Compare March 29, 2017 16:39

ijuma approved these changes Mar 30, 2017

View reviewed changes

asfgit closed this in d345d53 Mar 30, 2017

cmccabe deleted the KAFKA-4902 branch May 20, 2019 18:40

KAFKA-4902: Utils#delete should correctly handle I/O errors and symlinks #2691

KAFKA-4902: Utils#delete should correctly handle I/O errors and symlinks #2691

Conversation

cmccabe commented Mar 15, 2017

asfbot commented Mar 15, 2017

asfbot commented Mar 15, 2017

asfbot commented Mar 15, 2017

asfbot commented Mar 15, 2017

asfbot commented Mar 15, 2017

asfbot commented Mar 15, 2017

cmccabe commented Mar 16, 2017

asfbot commented Mar 16, 2017

asfbot commented Mar 16, 2017

asfbot commented Mar 16, 2017

asfbot commented Mar 20, 2017

asfbot commented Mar 20, 2017

asfbot commented Mar 20, 2017

junrao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma Mar 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asfbot commented Mar 27, 2017

asfbot commented Mar 27, 2017

asfbot commented Mar 27, 2017

cmccabe commented Mar 28, 2017

asfbot commented Mar 28, 2017

asfbot commented Mar 28, 2017

asfbot commented Mar 28, 2017

asfbot commented Mar 28, 2017

asfbot commented Mar 28, 2017

asfbot commented Mar 28, 2017

ijuma commented Mar 29, 2017

cmccabe commented Mar 29, 2017

asfbot commented Mar 29, 2017

asfbot commented Mar 29, 2017

asfbot commented Mar 29, 2017

ijuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma Mar 30, 2017 •

edited