Fix for Windows FileStore issue NIFI-3579 by PuspenduBanerjee · Pull Request #1580 · apache/nifi

PuspenduBanerjee · 2017-03-09T05:49:07Z

mosermw · 2017-03-13T19:16:57Z

...framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java

                final String containerName = container.getKey();

-                final long capacity = Files.getFileStore(container.getValue()).getTotalSpace();
+                final long capacity = container.getValue().toFile().getTotalSpace();


If there is a problem, FileStore.getTotalSpace() throws IOException but File.getTotalSpace() returns 0. I think you should throw an IOException if capacity == 0, in order to maintain similar behavior here.

I agree with you that exception should be thrown. I would prefer a RuntimeException with a message like

System returned total space of the partition for {reponame} is zero byte. Nifi can not create a zero sized FileSystemRepository

As Javadoc says that IOException signals that an I/O exception of some sort has occurred. This class is the general class of exceptions produced by failed or interrupted I/O operations.
Let me know your thought, then we can take a decision & change.

…ported volume or usable space is zero sized.

PuspenduBanerjee · 2017-03-13T20:33:33Z

@mosermw Incorporated your comments. please review. Once your review is approved , @joewitt or any other committer can help to get that merged.

mosermw · 2017-03-14T17:55:09Z

+1 looks good, passes checkstyle, runs in NiFi on Windows when creating content_repository on startup and when it already exists. I will squash and merge to master. Thanks @PuspenduBanerjee

markap14 · 2017-03-16T12:53:26Z

...framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java

-                final long capacity = Files.getFileStore(container.getValue()).getTotalSpace();
+                final long capacity = container.getValue().toFile().getTotalSpace();
+                if(capacity==0) {
+                    throw new RuntimeException("System returned total space of the partition for " + containerName + " is zero byte. Nifi can not create a zero sized FileSystemRepository");


@PuspenduBanerjee the issue that @mosermw brought up is a valid point, I believe. The idea here was to ensure that things are backward compatible. However, this is essentially converting an IOException to a RuntimeException, which is certainly not backward compatible, because the code that calls this method would have been forced to catch IOException but likely is not catching RuntimeException. I believe this needs to remain an IOException or not be thrown at all.

markap14 · 2017-03-16T12:55:49Z

...framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java

        }
+        long capacity = path.toFile().getTotalSpace();
+        if(capacity==0) {
+            throw new RuntimeException("System returned total space of the partition for " + containerName + " is zero byte. Nifi can not create a zero sized FileSystemRepository");


I have the same concern here as above. In the constructor above, it is likely not a huge deal since it's the constructor and if any Exception gets thrown, NiFi will fail to startup. However, here it is a much bigger concern, as this is called from a few different places where IOException is caught.

markap14 · 2017-03-16T12:57:22Z

...framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java

-        return Files.getFileStore(path).getUsableSpace();
+        long usableSpace=path.toFile().getUsableSpace();
+        if(usableSpace==0) {
+            throw new RuntimeException("System returned usable space of the partition for " + containerName + " is zero byte. Nifi can not create a zero sized FileSystemRepository");


We shouldn't be throwing an Exception here at all. If the content repository runs out of disk space, it will return 0. This is a very valid value and should not result in an Exception being thrown.

Hi @markap14 Let me share the thought process that I have. As per my understanding usableSpace==0 means the system is already in deep trouble and nothing is getting stored at your flow file repo. So, anyway we have faced write/flush error and either handled that or couldn't. Which signifies that checking or not checking zero usable space at this stage does not make any practical difference.
Secondly, for a 1GB usable storage the chance of hitting that Runtime exception is 1 in 1073.7 millions i.e. 9.31322575e-8% .
This comment is applicable for all 3 queries/concerns.
If you still think that we need to care about it, I am in.

After more thought, I agree with the concerns that @markap14 raised. I think the constructor can throw RuntimeException as it exists now. But I think the getContainerCapacity() should throw IOException and getContainerUsableSpace() should just return the 0.

@mosermw Could you please explain the usecases. Please look into my thought process that I have shared.

@PuspenduBanerjee if we run out of usable space then it's true that we won't be able to write to the content repository. As a result, processors will fail if they try. However, this is a completely unrelated concern. This is asking the repository how much space is available. This is used, for instance, to show the information in the UI, so that the user knows what is going on. If we throw an Exception here, now the user will be unable to see that there are 0 bytes available because an Exception is thrown so the HTTP request will return a 500 HTTP status code.

Hi @PuspenduBanerjee, since getContainerCapacity() and getContainerUsableSpace() are public methods, I would prefer that we try to maintain their existing contract. I would prefer to avoid even an extremely small chance of a RuntimeException.

PuspenduBanerjee · 2017-03-16T21:52:44Z

sure. I shall modify those and check in back. Thanks & Regards, Puspendu Banerjee

…

On Thu, Mar 16, 2017 at 11:59 AM, Michael Moser ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In nifi-nar-bundles/nifi-framework-bundle/nifi- framework/nifi-framework-core/src/main/java/org/apache/nifi/ controller/repository/FileSystemRepository.java <#1580 (comment)>: > @@ -392,8 +399,11 @@ public long getContainerUsableSpace(String containerName) throws IOException { if (path == null) { throw new IllegalArgumentException("No container exists with name " + containerName); } - - return Files.getFileStore(path).getUsableSpace(); + long usableSpace=path.toFile().getUsableSpace(); + if(usableSpace==0) { + throw new RuntimeException("System returned usable space of the partition for " + containerName + " is zero byte. Nifi can not create a zero sized FileSystemRepository"); Hi @PuspenduBanerjee <https://github.com/PuspenduBanerjee>, since getContainerCapacity() and getContainerUsableSpace() are public methods, I would prefer that we try to maintain their existing contract. I would prefer to avoid even an extremely small chance of a RuntimeException. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1580 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE2raFwemv-xHZFjHDmD_HfPuAoGZvW4ks5rmWpegaJpZM4MXrCF> .

PuspenduBanerjee · 2017-03-17T19:04:50Z

@mosermw & @markap14 please look into the updated PR#1602

Signed-off-by: Mike Moser <mosermw@apache.org> This closes apache#1580

PuspenduBanerjee added 2 commits March 8, 2017 23:32

Fix for Windows FileStore issue NiFi-3579

52877c0

checkstyle fixes

4fac610

mosermw reviewed Mar 13, 2017

View reviewed changes

incorporated review comments for throwing exception in case of the re…

eb895fc

…ported volume or usable space is zero sized.

asfgit closed this in 78382c6 Mar 14, 2017

markap14 reviewed Mar 16, 2017

View reviewed changes

josephxsxn pushed a commit to josephxsxn/nifi that referenced this pull request Mar 23, 2017

NIFI-3579 Fix for Windows FileStore issue

e6cbfb8

Signed-off-by: Mike Moser <mosermw@apache.org> This closes apache#1580

aperepel pushed a commit to aperepel/nifi that referenced this pull request Mar 29, 2017

NIFI-3579 Fix for Windows FileStore issue

da36c13

Signed-off-by: Mike Moser <mosermw@apache.org> This closes apache#1580

Conversation

PuspenduBanerjee commented Mar 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PuspenduBanerjee Mar 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PuspenduBanerjee commented Mar 13, 2017

Uh oh!

mosermw commented Mar 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PuspenduBanerjee Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PuspenduBanerjee commented Mar 16, 2017 via email

Uh oh!

PuspenduBanerjee commented Mar 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PuspenduBanerjee Mar 13, 2017 •

edited

Loading

PuspenduBanerjee Mar 16, 2017 •

edited

Loading

PuspenduBanerjee commented Mar 17, 2017 •

edited

Loading