storm-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From bo...@apache.org
Subject [1/2] storm git commit: STORM-3170: Fixed bug to eliminate invalid file deletion
Date Fri, 03 Aug 2018 16:12:28 GMT
Repository: storm
Updated Branches:
  refs/heads/master 674a7cb4a -> d64463609


STORM-3170: Fixed bug to eliminate invalid file deletion

with trivial refactoring


Project: http://git-wip-us.apache.org/repos/asf/storm/repo
Commit: http://git-wip-us.apache.org/repos/asf/storm/commit/fe94434a
Tree: http://git-wip-us.apache.org/repos/asf/storm/tree/fe94434a
Diff: http://git-wip-us.apache.org/repos/asf/storm/diff/fe94434a

Branch: refs/heads/master
Commit: fe94434a48f4f5e4f3812f6787117fa2b57e6d5d
Parents: 146beff
Author: Zhengdai Hu <hu.zhengdai@gmail.com>
Authored: Wed Aug 1 16:28:53 2018 -0500
Committer: Zhengdai Hu <hu.zhengdai@gmail.com>
Committed: Fri Aug 3 10:43:07 2018 -0500

----------------------------------------------------------------------
 .../logviewer/utils/DirectoryCleaner.java       | 102 +++++++++----------
 .../testsupport/MockRemovableFileBuilder.java   |  29 ++++++
 .../daemon/logviewer/utils/LogCleanerTest.java  |  29 +++---
 3 files changed, 92 insertions(+), 68 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/storm/blob/fe94434a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/DirectoryCleaner.java
----------------------------------------------------------------------
diff --git a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/DirectoryCleaner.java
b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/DirectoryCleaner.java
index bc8ae14..310bc8e 100644
--- a/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/DirectoryCleaner.java
+++ b/storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/DirectoryCleaner.java
@@ -24,13 +24,14 @@ import java.nio.file.DirectoryStream;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
-import java.util.Comparator;
+import java.util.HashSet;
 import java.util.List;
 import java.util.PriorityQueue;
 import java.util.Set;
 import java.util.Stack;
 import java.util.regex.Pattern;
 
+import org.apache.storm.utils.Utils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -93,31 +94,22 @@ public class DirectoryCleaner {
             return deletedFiles;
         }
 
-        Comparator<File> comparator = new Comparator<File>() {
-            public int compare(File f1, File f2) {
-                if (f1.lastModified() > f2.lastModified()) {
-                    return -1;
-                } else {
-                    return 1;
-                }
-            }
-        };
         // the oldest pq_size files in this directory will be placed in PQ, with the newest
at the root
-        PriorityQueue<File> pq = new PriorityQueue<File>(PQ_SIZE, comparator);
+        PriorityQueue<File> pq = new PriorityQueue<>(PQ_SIZE, (f1, f2) ->
f1.lastModified() > f2.lastModified() ? -1 : 1);
         int round = 0;
+        final Set<File> excluded = new HashSet<>();
         while (toDeleteSize > 0) {
             LOG.debug("To delete size is {}, start a new round of deletion, round: {}", toDeleteSize,
round);
             for (File dir : dirs) {
                 try (DirectoryStream<Path> stream = getStreamForDirectory(dir)) {
                     for (Path path : stream) {
                         File file = path.toFile();
-                        if (isFileEligibleToSkipDelete(forPerDir, activeDirs, dir, file))
{
-                            continue;
-                        }
-                        if (pq.size() < PQ_SIZE) {
-                            pq.offer(file);
-                        } else {
-                            if (file.lastModified() < pq.peek().lastModified()) {
+                        if (!excluded.contains(file)) {
+                            if (isFileEligibleToSkipDelete(forPerDir, activeDirs, dir, file))
{
+                                excluded.add(file);
+                            } else if (pq.size() < PQ_SIZE) {
+                                pq.offer(file);
+                            } else if (file.lastModified() < pq.peek().lastModified())
{
                                 pq.poll();
                                 pq.offer(file);
                             }
@@ -125,31 +117,44 @@ public class DirectoryCleaner {
                     }
                 }
             }
-            // need to reverse the order of elements in PQ to delete files from oldest to
newest
-            Stack<File> stack = new Stack<File>();
-            while (!pq.isEmpty()) {
-                File file = pq.poll();
-                stack.push(file);
-            }
-            while (!stack.isEmpty() && toDeleteSize > 0) {
-                File file = stack.pop();
-                toDeleteSize -= file.length();
-                LOG.info("Delete file: {}, size: {}, lastModified: {}", file.getCanonicalPath(),
file.length(), file.lastModified());
-                file.delete();
-                deletedFiles++;
-            }
-            pq.clear();
-            round++;
-            if (round >= MAX_ROUNDS) {
-                if (forPerDir) {
-                    LOG.warn("Reach the MAX_ROUNDS: {} during per-dir deletion, you may have
too many files in "
-                                    + "a single directory : {}, will delete the rest files
in next interval.",
+            if (!pq.isEmpty()) {
+                // need to reverse the order of elements in PQ to delete files from oldest
to newest
+                Stack<File> stack = new Stack<>();
+                while (!pq.isEmpty()) {
+                    File file = pq.poll();
+                    stack.push(file);
+                }
+                while (!stack.isEmpty() && toDeleteSize > 0) {
+                    File file = stack.pop();
+                    final String canonicalPath = file.getCanonicalPath();
+                    final long fileSize = file.length();
+                    final long lastModified = file.lastModified();
+                    //Original implementation doesn't actually check if delete succeeded
or not.
+                    try {
+                        Utils.forceDelete(file.getPath());
+                        LOG.info("Delete file: {}, size: {}, lastModified: {}", canonicalPath,
fileSize, lastModified);
+                        toDeleteSize -= fileSize;
+                        deletedFiles++;
+                    } catch (IOException e) {
+                        excluded.add(file);
+                    }
+                }
+                pq.clear();
+                round++;
+                if (round >= MAX_ROUNDS) {
+                    if (forPerDir) {
+                        LOG.warn("Reach the MAX_ROUNDS: {} during per-dir deletion, you may
have too many files in "
+                                + "a single directory : {}, will delete the rest files in
next interval.",
                             MAX_ROUNDS, dirs.get(0).getCanonicalPath());
-                } else {
-                    LOG.warn("Reach the MAX_ROUNDS: {} during global deletion, you may have
too many files, "
+                    } else {
+                        LOG.warn("Reach the MAX_ROUNDS: {} during global deletion, you may
have too many files, "
                             + "will delete the rest files in next interval.", MAX_ROUNDS);
+                    }
+                    break;
                 }
-                break;
+            } else {
+                LOG.warn("No more files able to delete this round, but {} is over quota by
{} MB",
+                    forPerDir ? "this directory" : "root directory", toDeleteSize * 1e-6);
             }
         }
         return deletedFiles;
@@ -157,21 +162,12 @@ public class DirectoryCleaner {
 
     private boolean isFileEligibleToSkipDelete(boolean forPerDir, Set<String> activeDirs,
File dir, File file) throws IOException {
         if (forPerDir) {
-            if (ACTIVE_LOG_PATTERN.matcher(file.getName()).matches()) {
-                return true;
-            }
+            return ACTIVE_LOG_PATTERN.matcher(file.getName()).matches();
         } else { // for global cleanup
-            if (activeDirs.contains(dir.getCanonicalPath())) { // for an active worker's
dir, make sure for the last "/"
-                if (ACTIVE_LOG_PATTERN.matcher(file.getName()).matches()) {
-                    return true;
-                }
-            } else {
-                if (META_LOG_PATTERN.matcher(file.getName()).matches()) {
-                    return true;
-                }
-            }
+            // for an active worker's dir, make sure for the last "/"
+            return activeDirs.contains(dir.getCanonicalPath()) ? ACTIVE_LOG_PATTERN.matcher(file.getName()).matches()
:
+                META_LOG_PATTERN.matcher(file.getName()).matches();
         }
-        return false;
     }
 
     /**

http://git-wip-us.apache.org/repos/asf/storm/blob/fe94434a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/testsupport/MockRemovableFileBuilder.java
----------------------------------------------------------------------
diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/testsupport/MockRemovableFileBuilder.java
b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/testsupport/MockRemovableFileBuilder.java
new file mode 100644
index 0000000..31204ce
--- /dev/null
+++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/testsupport/MockRemovableFileBuilder.java
@@ -0,0 +1,29 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one or more contributor license
agreements.
+ * See the NOTICE file distributed with this work for additional information regarding copyright
ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.  You may obtain a copy
of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and limitations under
the License.
+ */
+
+package org.apache.storm.daemon.logviewer.testsupport;
+
+import org.mockito.Mockito;
+
+import java.io.File;
+
+public class MockRemovableFileBuilder extends MockFileBuilder {
+    @Override
+    public File build() {
+        File mockFile = super.build();
+        Mockito.when(mockFile.delete()).thenReturn(true);
+        Mockito.when(mockFile.exists()).thenReturn(true);
+        return mockFile;
+    }
+}

http://git-wip-us.apache.org/repos/asf/storm/blob/fe94434a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
----------------------------------------------------------------------
diff --git a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
index 847edb1..491de54 100644
--- a/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
+++ b/storm-webapp/src/test/java/org/apache/storm/daemon/logviewer/utils/LogCleanerTest.java
@@ -26,11 +26,9 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyBoolean;
-import static org.mockito.ArgumentMatchers.anyList;
 import static org.mockito.ArgumentMatchers.anyListOf;
 import static org.mockito.ArgumentMatchers.anyLong;
 import static org.mockito.ArgumentMatchers.anyMapOf;
-import static org.mockito.ArgumentMatchers.anySet;
 import static org.mockito.ArgumentMatchers.anySetOf;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.Mockito.doAnswer;
@@ -54,7 +52,7 @@ import java.util.SortedSet;
 import java.util.TreeSet;
 
 import org.apache.storm.daemon.logviewer.testsupport.MockDirectoryBuilder;
-import org.apache.storm.daemon.logviewer.testsupport.MockFileBuilder;
+import org.apache.storm.daemon.logviewer.testsupport.MockRemovableFileBuilder;
 import org.apache.storm.daemon.supervisor.SupervisorUtils;
 import org.apache.storm.generated.LSWorkerHeartbeat;
 import org.apache.storm.utils.Time;
@@ -96,10 +94,10 @@ public class LogCleanerTest {
         matchingFiles.add(new MockDirectoryBuilder().setDirName("7077").setMtime(oldMtimeMillis).build());
 
         List<File> excludedFiles = new ArrayList<>();
-        excludedFiles.add(new MockFileBuilder().setFileName("oldlog-1-2-worker-.log").setMtime(oldMtimeMillis).build());
-        excludedFiles.add(new MockFileBuilder().setFileName("newlog-1-2-worker-.log").setMtime(newMtimeMillis).build());
-        excludedFiles.add(new MockFileBuilder().setFileName("some-old-file.txt").setMtime(oldMtimeMillis).build());
-        excludedFiles.add(new MockFileBuilder().setFileName("olddir-1-2-worker.log").setMtime(newMtimeMillis).build());
+        excludedFiles.add(new MockRemovableFileBuilder().setFileName("oldlog-1-2-worker-.log").setMtime(oldMtimeMillis).build());
+        excludedFiles.add(new MockRemovableFileBuilder().setFileName("newlog-1-2-worker-.log").setMtime(newMtimeMillis).build());
+        excludedFiles.add(new MockRemovableFileBuilder().setFileName("some-old-file.txt").setMtime(oldMtimeMillis).build());
+        excludedFiles.add(new MockRemovableFileBuilder().setFileName("olddir-1-2-worker.log").setMtime(newMtimeMillis).build());
         excludedFiles.add(new MockDirectoryBuilder().setDirName("metadata").setMtime(newMtimeMillis).build());
         excludedFiles.add(new MockDirectoryBuilder().setDirName("newdir").setMtime(newMtimeMillis).build());
 
@@ -130,13 +128,13 @@ public class LogCleanerTest {
 
             long nowMillis = Time.currentTimeMillis();
 
-            List<File> files1 = Seq.range(0, 10).map(idx -> new MockFileBuilder().setFileName("A"
+ idx)
+            List<File> files1 = Seq.range(0, 10).map(idx -> new MockRemovableFileBuilder().setFileName("A"
+ idx)
                     .setMtime(nowMillis + (100 * idx)).setLength(200).build())
                     .collect(toList());
-            List<File> files2 = Seq.range(0, 10).map(idx -> new MockFileBuilder().setFileName("B"
+ idx)
+            List<File> files2 = Seq.range(0, 10).map(idx -> new MockRemovableFileBuilder().setFileName("B"
+ idx)
                     .setMtime(nowMillis + (100 * idx)).setLength(200).build())
                     .collect(toList());
-            List<File> files3 = Seq.range(0, 10).map(idx -> new MockFileBuilder().setFileName("C"
+ idx)
+            List<File> files3 = Seq.range(0, 10).map(idx -> new MockRemovableFileBuilder().setFileName("C"
+ idx)
                     .setMtime(nowMillis + (100 * idx)).setLength(200).build())
                     .collect(toList());
             File port1Dir = new MockDirectoryBuilder().setDirName("/workers-artifacts/topo1/port1")
@@ -188,13 +186,13 @@ public class LogCleanerTest {
 
             long nowMillis = Time.currentTimeMillis();
 
-            List<File> files1 = Seq.range(0, 10).map(idx -> new MockFileBuilder().setFileName("A"
+ idx + ".log")
+            List<File> files1 = Seq.range(0, 10).map(idx -> new MockRemovableFileBuilder().setFileName("A"
+ idx + ".log")
                     .setMtime(nowMillis + (100 * idx)).setLength(200).build())
                     .collect(toList());
-            List<File> files2 = Seq.range(0, 10).map(idx -> new MockFileBuilder().setFileName("B"
+ idx)
+            List<File> files2 = Seq.range(0, 10).map(idx -> new MockRemovableFileBuilder().setFileName("B"
+ idx)
                     .setMtime(nowMillis + (100 * idx)).setLength(200).build())
                     .collect(toList());
-            List<File> files3 = Seq.range(0, 10).map(idx -> new MockFileBuilder().setFileName("C"
+ idx)
+            List<File> files3 = Seq.range(0, 10).map(idx -> new MockRemovableFileBuilder().setFileName("C"
+ idx)
                     .setMtime(nowMillis + (100 * idx)).setLength(200).build())
                     .collect(toList());
 
@@ -283,8 +281,8 @@ public class LogCleanerTest {
      */
     @Test
     public void testCleanupFn() throws IOException {
-        File mockFile1 = new MockFileBuilder().setFileName("delete-me1").build();
-        File mockFile2 = new MockFileBuilder().setFileName("delete-me2").build();
+        File mockFile1 = new MockRemovableFileBuilder().setFileName("delete-me1").build();
+        File mockFile2 = new MockRemovableFileBuilder().setFileName("delete-me2").build();
 
         Utils prevUtils = null;
         try {
@@ -311,6 +309,7 @@ public class LogCleanerTest {
                 @Override
                 SortedSet<File> getDeadWorkerDirs(int nowSecs, Set<File> logDirs)
throws Exception {
                     SortedSet<File> dirs = new TreeSet<>();
+                    //Test maybe flawed, as those weren't actually mocked dirs but mocked
regular files
                     dirs.add(mockFile1);
                     dirs.add(mockFile2);
                     return dirs;


Mime
View raw message