[GitHub] storm pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
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...
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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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 pull request #2788: STORM-3170: Fixed bug to eliminate invalid file de...
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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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...
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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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...
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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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...
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...
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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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...
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
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * 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...
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...
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 Date: 2018-08-01T21:28:53Z STORM-3170: Fixed bug to eliminate invalid file deletion with trivial refactoring ---
