You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by zd-project <gi...@git.apache.org> on 2018/08/01 21:29:39 UTC
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
GitHub user zd-project opened a pull request:
https://github.com/apache/storm/pull/2788
STORM-3170: Fixed bug to eliminate invalid file deletion
https://issues.apache.org/jira/browse/STORM-3170
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zd-project/storm STORM-3170
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2788.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2788
----
commit 8f69c241b90b00205cae195be990795c44fb7a96
Author: Zhengdai Hu <hu...@...>
Date: 2018-08-01T21:28:53Z
STORM-3170: Fixed bug to eliminate invalid file deletion
with trivial refactoring
----
---
[GitHub] storm issue #2788: STORM-3170: Fixed bug to eliminate invalid file deletion
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the issue:
https://github.com/apache/storm/pull/2788
Still +1
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2788#discussion_r207327029
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/DirectoryCleaner.java ---
@@ -133,10 +123,15 @@ public int compare(File f1, File f2) {
}
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++;
+ 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.
+ if (file.delete()) {
--- End diff --
I agree that we should make this check, but won't this change make the outer while loop run again on the same files if the files couldn't be deleted?
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2788#discussion_r207587402
--- Diff: 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);
--- End diff --
No, it's just me nitpicking. We can always replace it if it becomes an issue to use in tests.
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on a diff in the pull request:
https://github.com/apache/storm/pull/2788#discussion_r207327774
--- Diff: 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);
--- End diff --
This stubbing is a little weird. It'll make the files pretend to be deleteable, but once you delete the file it'll still say it exists, and calling delete again will still return true.
Is the reason we're mocking File rather than creating temporary files for performance reasons?
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2788#discussion_r207328701
--- Diff: 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);
--- End diff --
I'm simply leveraging what's already out there. Maybe it's easier to control the behavior of a file in this way? I'll look into the implementation to see if I can alter behavior after calling `#delete`
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2788
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2788#discussion_r207376890
--- Diff: storm-webapp/src/main/java/org/apache/storm/daemon/logviewer/utils/DirectoryCleaner.java ---
@@ -133,10 +123,15 @@ public int compare(File f1, File f2) {
}
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++;
+ 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.
+ if (file.delete()) {
--- End diff --
I'm wondering if we should use forcedelete here directly?
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2788#discussion_r207586437
--- Diff: 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);
--- End diff --
Should I still concern about this issue then? @revans2 @srdo
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2788#discussion_r207539609
--- Diff: 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);
--- End diff --
@zd-project you can control it more fully, but it gets a little more complicated. Not a big deal but if it is causing problems it can be done.
---
[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on a diff in the pull request:
https://github.com/apache/storm/pull/2788#discussion_r207587787
--- Diff: 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);
--- End diff --
Okay I'll just file a minor jira then
---
[GitHub] storm issue #2788: STORM-3170: Fixed bug to eliminate invalid file deletion
Posted by srdo <gi...@git.apache.org>.
Github user srdo commented on the issue:
https://github.com/apache/storm/pull/2788
+1, thanks for fixing the issue with nondeleteable files. The solution looks good. Please squash and we can merge.
---
[GitHub] storm issue #2788: STORM-3170: Fixed bug to eliminate invalid file deletion
Posted by zd-project <gi...@git.apache.org>.
Github user zd-project commented on the issue:
https://github.com/apache/storm/pull/2788
This will affect #2754
---