You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@storm.apache.org by bo...@apache.org on 2018/08/03 16:12:28 UTC
[1/2] storm git commit: STORM-3170: Fixed bug to eliminate invalid
file deletion
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...@gmail.com>
Authored: Wed Aug 1 16:28:53 2018 -0500
Committer: Zhengdai Hu <hu...@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;
[2/2] storm git commit: Merge branch 'STORM-3170' of
https://github.com/zd-project/storm into STORM-3170
Posted by bo...@apache.org.
Merge branch 'STORM-3170' of https://github.com/zd-project/storm into STORM-3170
STORM-3170: Fixed bug to eliminate invalid file deletion
This closes #2788
Project: http://git-wip-us.apache.org/repos/asf/storm/repo
Commit: http://git-wip-us.apache.org/repos/asf/storm/commit/d6446360
Tree: http://git-wip-us.apache.org/repos/asf/storm/tree/d6446360
Diff: http://git-wip-us.apache.org/repos/asf/storm/diff/d6446360
Branch: refs/heads/master
Commit: d644636096c37fb7cace1b6823453d398f0a7024
Parents: 674a7cb fe94434
Author: Robert Evans <ev...@yahoo-inc.com>
Authored: Fri Aug 3 10:49:17 2018 -0500
Committer: Robert Evans <ev...@yahoo-inc.com>
Committed: Fri Aug 3 10:49:17 2018 -0500
----------------------------------------------------------------------
.../logviewer/utils/DirectoryCleaner.java | 102 +++++++++----------
.../testsupport/MockRemovableFileBuilder.java | 29 ++++++
.../daemon/logviewer/utils/LogCleanerTest.java | 29 +++---
3 files changed, 92 insertions(+), 68 deletions(-)
----------------------------------------------------------------------