STORM-3170: Fixed bug to eliminate invalid file deletion #2788
Conversation
|
This will affect #2754 |
| final long fileSize = file.length(); | ||
| final long lastModified = file.lastModified(); | ||
| //Original implementation doesn't actually check if delete succeeded or not. | ||
| if (file.delete()) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm wondering if we should use forcedelete here directly?
| @Override | ||
| public File build() { | ||
| File mockFile = super.build(); | ||
| Mockito.when(mockFile.delete()).thenReturn(true); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
No, it's just me nitpicking. We can always replace it if it becomes an issue to use in tests.
There was a problem hiding this comment.
Okay I'll just file a minor jira then
|
+1, thanks for fixing the issue with nondeleteable files. The solution looks good. Please squash and we can merge. |
with trivial refactoring
|
Still +1 |
… STORM-3170 STORM-3170: Fixed bug to eliminate invalid file deletion This closes #2788
https://issues.apache.org/jira/browse/STORM-3170