[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local resources by RuiLi8080 · Pull Request #3226 · apache/storm

RuiLi8080 · 2020-03-12T14:37:15Z

Supervisor relies on AsyncLocalizer to download blobs from blob store.
AsyncLocalizer uses downloadService pool to process CompletableFuture objects in parallel.

We have noticed a case that while the downloading task is waiting for a thread to execute, new assignment changes will try to release the slot by dereferences all of the related local resources.

However, reading local resources assumes two base blob downloading task have been completed which is not always true.

RuiLi8080 · 2020-03-12T14:38:05Z

Referring to #3210

agresch · 2020-03-12T14:56:07Z

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

agresch · 2020-03-12T14:46:52Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

+            // 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. "


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?

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.

agresch · 2020-03-12T16:14:43Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

        } 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);


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.

Good suggestion, added.

agresch · 2020-03-12T16:30:09Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

        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");


spelling of supervisor.

Also please update ClusterMetrics.md with this metric and any other missing that are in this file.

Thanks for catching, updated

agresch

please squash

… resources address comments add file not found meter fix typo and add documentation

RuiLi8080 · 2020-03-13T02:42:41Z

@agresch Squashed

Ethanlm · 2020-03-13T14:46:49Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

-        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


This is our corp internal commit and should be changed to community JIRA.

Ethanlm · 2020-03-13T14:49:14Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

+        } catch (FileNotFoundException fnfException) {
+            localResourceFileNotFound.mark();
+            LOG.warn("Local base blobs have not been downloaded yet. "
+                        + "DownloadExecService is too busy", fnfException);


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.

Make sense. Removed

Ethanlm · 2020-03-13T14:53:00Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

+        for (LocalResource lr : localResources) {
            try {
                removeBlobReference(lr.getBlobName(), pna, lr.shouldUncompress());
            } catch (Exception e) {


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

Ethanlm · 2020-03-13T15:01:58Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

+            return;
+        } catch (IOException ioException) {
+            LOG.error("Unable to read local conf file. ", ioException);
+            LOG.info("Port and assignment info: {}", pna);


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 }

Ethanlm · 2020-03-13T15:04:35Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

        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");


This is very specific to local resource file not found inside releaseSlotFor. So the description and the variable should be more specific.

Ethanlm · 2020-03-13T15:10:27Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java


-        for (LocalResource lr : getLocalResources(pna)) {
+
+        // ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService


should have been

Ethanlm · 2020-03-16T17:22:33Z

storm-server/src/main/java/org/apache/storm/localizer/AsyncLocalizer.java

+            // 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) {


This can be IOException

Ethanlm

+1

agresch reviewed Mar 12, 2020

View reviewed changes

agresch approved these changes Mar 12, 2020

View reviewed changes

[STORM-3583] Handle exceptions when AsyncLocalizer tries to get local…

17c17e3

… resources address comments add file not found meter fix typo and add documentation

RuiLi8080 force-pushed the STORM-3583 branch from af189fa to 17c17e3 Compare March 13, 2020 02:40

Ethanlm reviewed Mar 13, 2020

View reviewed changes

Ethanlm reviewed Mar 16, 2020

View reviewed changes

addressed comments

e36e28a

RuiLi8080 force-pushed the STORM-3583 branch from 6938123 to e36e28a Compare March 17, 2020 04:52

Ethanlm approved these changes Mar 23, 2020

View reviewed changes

Ethanlm merged commit ec217df into apache:master Mar 23, 2020


		for (LocalResource lr : getLocalResources(pna)) {

		// ALERT: A possible race condition could be resolved by separating the thread pools into downloadExecService and taskExecService

Conversation

RuiLi8080 commented Mar 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RuiLi8080 commented Mar 12, 2020

Uh oh!

agresch commented Mar 12, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

agresch Mar 12, 2020 • 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!

agresch left a comment

Choose a reason for hiding this comment

Uh oh!

RuiLi8080 commented Mar 13, 2020

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!

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!

Ethanlm Mar 13, 2020 • 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!

Ethanlm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

RuiLi8080 commented Mar 12, 2020 •

edited

Loading

agresch Mar 12, 2020 •

edited

Loading

Ethanlm Mar 13, 2020 •

edited

Loading