You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by arunmahadevan <gi...@git.apache.org> on 2018/10/11 17:35:17 UTC
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
GitHub user arunmahadevan opened a pull request:
https://github.com/apache/storm/pull/2871
[STORM-3252] Bug fix for blobstore sync
1.Bug fix for blob sync frequency with time unit error.
2.Bug fix for blob sync delete file, add catch NoSuchFileException.
3.Bug fix for blob sync update blob flie, add catch FileNotFoundException
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arunmahadevan/storm STORM-3252
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2871.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 #2871
----
commit 94e6bbcc873c04034c55977653910b9921bc014c
Author: jiangzhileaf <ji...@...>
Date: 2018-07-19T08:37:22Z
Bug fix for blobstore sync.
1.Bug fix for blob sync frequency with time unit error.
2.Bug fix for blob sync delete file, add catch NoSuchFileException.
3.Bug fix for blob sync update blob flie, add catch FileNotFoundException
----
---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224541248
--- Diff: storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map<String, Object> conf, BlobStore bl
out.close();
}
isSuccess = true;
+ } catch(FileNotFoundException fnf) {
+ LOG.warn("FileNotFoundException", fnf);
--- End diff --
It crashes the nimbus if the exception is propagated. (we have seen this happening if topology gets killed while the non-leader nimbus tries to download the blob). It may be better to swallow and let `downloadUpdatedBlob` return false here?
---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224548982
--- Diff: storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map<String, Object> conf, BlobStore bl
out.close();
}
isSuccess = true;
+ } catch(FileNotFoundException fnf) {
+ LOG.warn("FileNotFoundException", fnf);
--- End diff --
I see. but then, any other `IOException` such as failure to reach Blobstore etc could cause nimbus restart. I would treat all IOExceptions in same manner. i.e. log them and return `false`.
---
[GitHub] storm issue #2871: [STORM-3252] Bug fix for blobstore sync
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on the issue:
https://github.com/apache/storm/pull/2871
> @kishorvpatil I am not sure if we want to swallow IOException, since it may be due to some serious problem which we want to propagate and nimbus cannot continue in that case.
Agreed. We don't want any other `IOException`.
---
[GitHub] storm issue #2871: [STORM-3252] Bug fix for blobstore sync
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the issue:
https://github.com/apache/storm/pull/2871
@revans2 , updated the log message, please check if it makes sense.
@kishorvpatil I am not sure if we want to swallow IOException, since it may be due to some serious problem which we want to propagate and nimbus cannot continue in that case.
---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Posted by kishorvpatil <gi...@git.apache.org>.
Github user kishorvpatil commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224538909
--- Diff: storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map<String, Object> conf, BlobStore bl
out.close();
}
isSuccess = true;
+ } catch(FileNotFoundException fnf) {
+ LOG.warn("FileNotFoundException", fnf);
--- End diff --
rethrow fnf?
---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224558607
--- Diff: storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map<String, Object> conf, BlobStore bl
out.close();
}
isSuccess = true;
+ } catch(FileNotFoundException fnf) {
+ LOG.warn("Blobstore file for key '%s' does not exist or got deleted before it could be downloaded.", key, fnf);
--- End diff --
I think you want '{}' instead of '%s'
---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224550336
--- Diff: storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map<String, Object> conf, BlobStore bl
out.close();
}
isSuccess = true;
+ } catch(FileNotFoundException fnf) {
+ LOG.warn("FileNotFoundException", fnf);
--- End diff --
Okay I think I see it now. On the local file output stream it can show up if the file/directory is deleted before we do the commit. It would be good to document this, preferably in the LOG message, because FileNotFoundException is redundant, it is already going to log that and knowing why this is only a warning is preferable.
---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224559083
--- Diff: storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map<String, Object> conf, BlobStore bl
out.close();
}
isSuccess = true;
+ } catch(FileNotFoundException fnf) {
+ LOG.warn("Blobstore file for key '%s' does not exist or got deleted before it could be downloaded.", key, fnf);
--- End diff --
Thanks for catching.
---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2871
---
[GitHub] storm issue #2871: [STORM-3252] Bug fix for blobstore sync
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on the issue:
https://github.com/apache/storm/pull/2871
Cherry picked commits from https://github.com/apache/storm/pull/2773 and linked with a JIRA.
---
[GitHub] storm pull request #2871: [STORM-3252] Bug fix for blobstore sync
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on a diff in the pull request:
https://github.com/apache/storm/pull/2871#discussion_r224547803
--- Diff: storm-server/src/main/java/org/apache/storm/blobstore/BlobStoreUtils.java ---
@@ -191,6 +192,8 @@ public static boolean downloadUpdatedBlob(Map<String, Object> conf, BlobStore bl
out.close();
}
isSuccess = true;
+ } catch(FileNotFoundException fnf) {
+ LOG.warn("FileNotFoundException", fnf);
--- End diff --
Line 271 of this file is already dealing with the race. so in theory eating the FileNotFound or rethrowing it as a KeyNotFound exception should be fine. I am just trying to understand where the FileNotFound is coming from.
The remoteBlobStore should only be getting thrift exceptions. The only place in the code I see that could throw a FileNotFoundException is when we are interacting with the local blob, but even then the blobstore code should be changing it into a KeyNotFound or some other thrift exception.
---