You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by PuspenduBanerjee <gi...@git.apache.org> on 2017/03/09 05:49:12 UTC
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
GitHub user PuspenduBanerjee opened a pull request:
https://github.com/apache/nifi/pull/1580
Fix for Windows FileStore issue NIFI-3579
this fixes NIFI-3579.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/PuspenduBanerjee/nifi NIFI-3579
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/1580.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 #1580
----
commit 52877c0fee7479824bf6189d89818bfef919e6cc
Author: Puspendu Banerjee <pu...@gmail.com>
Date: 2017-03-09T05:26:43Z
Fix for Windows FileStore issue NiFi-3579
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r106435047
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -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");
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r106409739
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -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");
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1580: Fix for Windows FileStore issue NIFI-3579
Posted by PuspenduBanerjee <gi...@git.apache.org>.
Github user PuspenduBanerjee commented on the issue:
https://github.com/apache/nifi/pull/1580
sure. I shall modify those and check in back.
Thanks & Regards,
Puspendu Banerjee
On Thu, Mar 16, 2017 at 11:59 AM, Michael Moser <no...@github.com>
wrote:
> *@mosermw* 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
> <https://github.com/apache/nifi/pull/1580#discussion_r106473572>:
>
> > @@ -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.
>
> \u2014
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/nifi/pull/1580#discussion_r106473572>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AE2raFwemv-xHZFjHDmD_HfPuAoGZvW4ks5rmWpegaJpZM4MXrCF>
> .
>
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by PuspenduBanerjee <gi...@git.apache.org>.
Github user PuspenduBanerjee commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r106420146
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -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");
--- End diff --
Hi @markap14 Let me place share the thought process that I have. As per my understanding usableSpace==0 means the system is already at in deep trouble or 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 1073741824* 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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r105747874
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -199,13 +199,13 @@ public FileSystemRepository(final NiFiProperties nifiProperties) throws IOExcept
for (final Map.Entry<String, Path> container : containers.entrySet()) {
final String containerName = container.getKey();
- final long capacity = Files.getFileStore(container.getValue()).getTotalSpace();
+ final long capacity = container.getValue().toFile().getTotalSpace();
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r106408789
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -199,13 +199,16 @@ public FileSystemRepository(final NiFiProperties nifiProperties) throws IOExcept
for (final Map.Entry<String, Path> container : containers.entrySet()) {
final String containerName = container.getKey();
- 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");
--- End diff --
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1580: Fix for Windows FileStore issue NIFI-3579
Posted by PuspenduBanerjee <gi...@git.apache.org>.
Github user PuspenduBanerjee commented on the issue:
https://github.com/apache/nifi/pull/1580
@mosermw Incorporated your comments. please review. Once your review is approved , @joewitt or any other committer can help to get that merged.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by PuspenduBanerjee <gi...@git.apache.org>.
Github user PuspenduBanerjee commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r105752431
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -199,13 +199,13 @@ public FileSystemRepository(final NiFiProperties nifiProperties) throws IOExcept
for (final Map.Entry<String, Path> container : containers.entrySet()) {
final String containerName = container.getKey();
- final long capacity = Files.getFileStore(container.getValue()).getTotalSpace();
+ final long capacity = container.getValue().toFile().getTotalSpace();
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r106409449
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -382,8 +385,12 @@ public long getContainerCapacity(final String containerName) throws IOException
if (path == null) {
throw new IllegalArgumentException("No container exists with name " + containerName);
}
+ 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");
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by PuspenduBanerjee <gi...@git.apache.org>.
Github user PuspenduBanerjee commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r106438150
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -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");
--- End diff --
@mosermw Could you please explain the usecases. Please look into my thought process that I have shared.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r106473572
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -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");
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by markap14 <gi...@git.apache.org>.
Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1580#discussion_r106460083
--- Diff: nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-framework-core/src/main/java/org/apache/nifi/controller/repository/FileSystemRepository.java ---
@@ -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");
--- End diff --
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1580: Fix for Windows FileStore issue NIFI-3579
Posted by mosermw <gi...@git.apache.org>.
Github user mosermw commented on the issue:
https://github.com/apache/nifi/pull/1580
+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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request #1580: Fix for Windows FileStore issue NIFI-3579
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/1580
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi issue #1580: Fix for Windows FileStore issue NIFI-3579
Posted by PuspenduBanerjee <gi...@git.apache.org>.
Github user PuspenduBanerjee commented on the issue:
https://github.com/apache/nifi/pull/1580
@mosermw & @markap14 please look into the updated [PR#1602|https://github.com/apache/nifi/pull/1602]
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---