-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
STORM-2215: validate blobs are present before submitting #1795
Conversation
@@ -587,4 +592,10 @@ private static double getMaxExecutorMemoryUsageForTopo(StormTopology topology, M | |||
} | |||
return largestMemoryOperator; | |||
} | |||
|
|||
private static Set<String> getListOfKeysFromBlobStore(Map<String, Object> stormConf) { | |||
NimbusBlobStore client = new NimbusBlobStore(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to guarantee calling client.shutdown().
@@ -1338,6 +1339,26 @@ public static void handleUncaughtException(Throwable t) { | |||
} | |||
} | |||
|
|||
public static void validateTopologyBlobStoreMap(Map stormConf, Set<String> blobStoreKeys) throws InvalidTopologyException { | |||
boolean containsAllBlobs = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: We're adding missing keys to Set so no need to have additional flag. missingKeys.size() > 0
is same to containsAllBlobs == false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found possibility of resource leak which needs to be checked but looks good to me otherwise.
@HeartSaVioR I rebased and addressed the review comments, but I want to merge in #1744 first and then I will update this one again. |
#1744 is in and this is rebased |
+1 Thanks for the work. |
No description provided.