You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by GitBox <gi...@apache.org> on 2020/03/12 14:37:16 UTC
[GitHub] [storm] RuiLi8080 opened a new pull request #3226: [STORM-3583]
Handle exceptions when AsyncLocalizer tries to get local resources
RuiLi8080 opened a new pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r393189955
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,13 +460,31 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
- try {
- removeBlobReference(lr.getBlobName(), pna, lr.shouldUncompress());
- } catch (Exception e) {
- throw new IOException(e);
+
+ // ALERT: A possible race condition should have been resolved
+ // by separating the thread pools into downloadExecService and taskExecService
+ // https://github.com/apache/storm/pull/3153
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (Exception e) {
Review comment:
This can be `IOException`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r391740828
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -457,17 +457,23 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- List<LocalResource> localResources = null;
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
try {
// Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
// Precondition2: Both these two blob files are fully downloaded and proper permission been set
localResources = getLocalResources(pna);
} catch (FileNotFoundException fnfException) {
LOG.warn("Local base blobs have not been downloaded yet. "
+ "DownloadExecService is too busy", fnfException);
+ LOG.info("Port and assignment info: {}", pna);
Review comment:
Good suggestion, added.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392706622
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392282651
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (FileNotFoundException fnfException) {
+ localResourceFileNotFound.mark();
+ LOG.warn("Local base blobs have not been downloaded yet. "
+ + "DownloadExecService is too busy", fnfException);
+ LOG.info("Port and assignment info: {}", pna);
+ return;
+ } catch (IOException ioException) {
+ LOG.error("Unable to read local conf file. ", ioException);
+ LOG.info("Port and assignment info: {}", pna);
Review comment:
`LOG.info("Port and assignment info: {}", pna); ` are repeated twice here. We can write it in the following way:
```
catch (IOException e) {
//common1
if (e instanceof FileNotFoundException) {
//xx
} else {
//yy
}
// common2
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392706708
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
Review comment:
Changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392274684
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (FileNotFoundException fnfException) {
+ localResourceFileNotFound.mark();
+ LOG.warn("Local base blobs have not been downloaded yet. "
+ + "DownloadExecService is too busy", fnfException);
Review comment:
I would remove this line. "too busy" can be misleading. The files not yet being fully downloaded could because it is just started to download.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392287727
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
Review comment:
`should have been `
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392277063
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (FileNotFoundException fnfException) {
+ localResourceFileNotFound.mark();
+ LOG.warn("Local base blobs have not been downloaded yet. "
+ + "DownloadExecService is too busy", fnfException);
+ LOG.info("Port and assignment info: {}", pna);
+ return;
+ } catch (IOException ioException) {
+ LOG.error("Unable to read local conf file. ", ioException);
+ LOG.info("Port and assignment info: {}", pna);
+ throw ioException;
+ }
+
+ for (LocalResource lr : localResources) {
try {
removeBlobReference(lr.getBlobName(), pna, lr.shouldUncompress());
} catch (Exception e) {
Review comment:
These exception handlings seems can be removed. But I am fine with leaving it there since it's not related to your change in this PR
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r393444039
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,13 +460,31 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
- try {
- removeBlobReference(lr.getBlobName(), pna, lr.shouldUncompress());
- } catch (Exception e) {
- throw new IOException(e);
+
+ // ALERT: A possible race condition should have been resolved
+ // by separating the thread pools into downloadExecService and taskExecService
+ // https://github.com/apache/storm/pull/3153
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (Exception e) {
Review comment:
Fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] agresch commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r391731839
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -457,17 +457,23 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- List<LocalResource> localResources = null;
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
try {
// Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
// Precondition2: Both these two blob files are fully downloaded and proper permission been set
localResources = getLocalResources(pna);
} catch (FileNotFoundException fnfException) {
LOG.warn("Local base blobs have not been downloaded yet. "
+ "DownloadExecService is too busy", fnfException);
+ LOG.info("Port and assignment info: {}", pna);
Review comment:
If we're preceding with this change, I think we should mark a meter so we can add an alert and investigate that this is occurring and evaluate.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392706690
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (FileNotFoundException fnfException) {
+ localResourceFileNotFound.mark();
+ LOG.warn("Local base blobs have not been downloaded yet. "
+ + "DownloadExecService is too busy", fnfException);
Review comment:
Make sense. Removed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r391737966
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +457,21 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+ List<LocalResource> localResources = null;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (FileNotFoundException fnfException) {
+ LOG.warn("Local base blobs have not been downloaded yet. "
Review comment:
Added pna info. As for the race condition, I think that could be addressed by separating executor pools. I did not think of a scenario that race condition will happen because of catching the exception.
All downloading tasks are CompletableFuture object. File no found happens when the object is not completed. So here we will forget about it, and dereference any resources we can, and release the slot for new assignments.
Then the background download threads will succeeds at some points. The cleanup thread will take care of these orphan files even if they are not released properly.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on issue #3226: [STORM-3583] Handle
exceptions when AsyncLocalizer tries to get local resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on issue #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#issuecomment-598223958
Referring to #3210
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] agresch commented on issue #3226: [STORM-3583] Handle
exceptions when AsyncLocalizer tries to get local resources
Posted by GitBox <gi...@apache.org>.
agresch commented on issue #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#issuecomment-598233987
I already made this same change and had to revert it:
Similar initial change:
https://git.ouroath.com/storm/storm/commit/6ff56893479266f3a5656bdf26c07d40a4e1ae98#diff-74535cb89e9e926ad424a8d1e2fa9586
Side effect this caused:
https://issues.apache.org/jira/browse/STORM-3523
revert:
https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] agresch commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r391742280
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -110,6 +111,7 @@
this.blobCacheUpdateDuration = metricsRegistry.registerTimer("supervisor:blob-cache-update-duration");
this.blobLocalizationDuration = metricsRegistry.registerTimer("supervisor:blob-localization-duration");
this.numBlobUpdateVersionChanged = metricsRegistry.registerMeter("supervisor:num-blob-update-version-changed");
+ this.localResourceFileNotFound = metricsRegistry.registerMeter("supervisdor:local-resource-file-not-found");
Review comment:
spelling of supervisor.
Also please update ClusterMetrics.md with this metric and any other missing that are in this file.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] agresch commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r391731839
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -457,17 +457,23 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- List<LocalResource> localResources = null;
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
try {
// Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
// Precondition2: Both these two blob files are fully downloaded and proper permission been set
localResources = getLocalResources(pna);
} catch (FileNotFoundException fnfException) {
LOG.warn("Local base blobs have not been downloaded yet. "
+ "DownloadExecService is too busy", fnfException);
+ LOG.info("Port and assignment info: {}", pna);
Review comment:
If we're proceeding with this change, I think we should mark a meter so we can add an alert and investigate that this is occurring and evaluate.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392706643
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (FileNotFoundException fnfException) {
+ localResourceFileNotFound.mark();
+ LOG.warn("Local base blobs have not been downloaded yet. "
+ + "DownloadExecService is too busy", fnfException);
+ LOG.info("Port and assignment info: {}", pna);
+ return;
+ } catch (IOException ioException) {
+ LOG.error("Unable to read local conf file. ", ioException);
+ LOG.info("Port and assignment info: {}", pna);
Review comment:
changed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392273215
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
Review comment:
This is our corp internal commit and should be changed to community JIRA.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r391798667
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -110,6 +111,7 @@
this.blobCacheUpdateDuration = metricsRegistry.registerTimer("supervisor:blob-cache-update-duration");
this.blobLocalizationDuration = metricsRegistry.registerTimer("supervisor:blob-localization-duration");
this.numBlobUpdateVersionChanged = metricsRegistry.registerMeter("supervisor:num-blob-update-version-changed");
+ this.localResourceFileNotFound = metricsRegistry.registerMeter("supervisdor:local-resource-file-not-found");
Review comment:
Thanks for catching, updated
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on issue #3226: [STORM-3583] Handle
exceptions when AsyncLocalizer tries to get local resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on issue #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#issuecomment-598520889
@agresch Squashed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] agresch commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
agresch commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r391672288
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +457,21 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+ List<LocalResource> localResources = null;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (FileNotFoundException fnfException) {
+ LOG.warn("Local base blobs have not been downloaded yet. "
Review comment:
can we add the pna to this message and the IOException error? What is the side effect of not releasing this if there's a race condition? Could the localizer then be updating this file continually?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392287727
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
Review comment:
`could have been `
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm merged pull request #3226: [STORM-3583] Handle
exceptions when AsyncLocalizer tries to get local resources
Posted by GitBox <gi...@apache.org>.
Ethanlm merged pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] Ethanlm commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
Ethanlm commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392284317
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -109,6 +111,7 @@
this.blobCacheUpdateDuration = metricsRegistry.registerTimer("supervisor:blob-cache-update-duration");
this.blobLocalizationDuration = metricsRegistry.registerTimer("supervisor:blob-localization-duration");
this.numBlobUpdateVersionChanged = metricsRegistry.registerMeter("supervisor:num-blob-update-version-changed");
+ this.localResourceFileNotFound = metricsRegistry.registerMeter("supervisor:local-resource-file-not-found");
Review comment:
This is very specific to local resource file not found inside `releaseSlotFor`. So the description and the variable should be more specific.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392706633
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -109,6 +111,7 @@
this.blobCacheUpdateDuration = metricsRegistry.registerTimer("supervisor:blob-cache-update-duration");
this.blobLocalizationDuration = metricsRegistry.registerTimer("supervisor:blob-localization-duration");
this.numBlobUpdateVersionChanged = metricsRegistry.registerMeter("supervisor:num-blob-update-version-changed");
+ this.localResourceFileNotFound = metricsRegistry.registerMeter("supervisor:local-resource-file-not-found");
Review comment:
fixed
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [storm] RuiLi8080 commented on a change in pull request #3226:
[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local
resources
Posted by GitBox <gi...@apache.org>.
RuiLi8080 commented on a change in pull request #3226: [STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources
URL: https://github.com/apache/storm/pull/3226#discussion_r392706656
##########
File path: storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java
##########
@@ -456,7 +459,28 @@ public void releaseSlotFor(LocalAssignment assignment, int port) throws IOExcept
topoConfBlob.removeReference(pna);
}
- for (LocalResource lr : getLocalResources(pna)) {
+
+ // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService
+ // https://git.ouroath.com/storm/storm/commit/ebd52b37c7448d381d31451e46e8f19c6e51352d#diff-74535cb89e9e926ad424a8d1e2fa9586
+ // Will need further investigation if the race condition happens again
+ List<LocalResource> localResources;
+ try {
+ // Precondition1: Base blob stormconf.ser and stormcode.ser have been localized
+ // Precondition2: Both these two blob files are fully downloaded and proper permission been set
+ localResources = getLocalResources(pna);
+ } catch (FileNotFoundException fnfException) {
+ localResourceFileNotFound.mark();
+ LOG.warn("Local base blobs have not been downloaded yet. "
+ + "DownloadExecService is too busy", fnfException);
+ LOG.info("Port and assignment info: {}", pna);
+ return;
+ } catch (IOException ioException) {
+ LOG.error("Unable to read local conf file. ", ioException);
+ LOG.info("Port and assignment info: {}", pna);
+ throw ioException;
+ }
+
+ for (LocalResource lr : localResources) {
try {
removeBlobReference(lr.getBlobName(), pna, lr.shouldUncompress());
} catch (Exception e) {
Review comment:
Removed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services