Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[NEMO-54] Handle remote data fetch failures due to executor removal #67

Merged
merged 8 commits into from Jul 3, 2018
Merged

[NEMO-54] Handle remote data fetch failures due to executor removal #67

merged 8 commits into from Jul 3, 2018

Conversation

johnyangk
Copy link
Contributor

JIRA: NEMO-54: Handle remote data fetch failures due to executor removal

Major changes:

  • Catch all Throwables in ParentTaskDataFetcher, wrap it with an IOException, and throw the IOException over to TaskExecutor
  • Handle CompleteableFuture failures by passing the throwable to TaskExecutor via iteratorQueue

Minor changes to note:

  • Introduce ClosableBlockingQueue#closeExceptionally, with the hope to propagate errors all the way up to ParentTaskDataFetcher

Tests for the changes:

  • ParentTaskDataFetcherTest

Other comments:

resolves NEMO-54

@johnyangk johnyangk self-assigned this Jul 2, 2018
@johnyangk johnyangk requested a review from seojangho July 2, 2018 11:12
seojangho
seojangho reviewed Jul 3, 2018
View reviewed changes
runtime/executor/src/main/java/edu/snu/nemo/runtime/executor/task/ParentTaskDataFetcher.java Outdated
} catch (InterruptedException exception) {
Thread.currentThread().interrupt();
throw new IOException(exception);
} catch (Throwable e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final Throwable e

@johnyangk
Copy link
Contributor Author

Thanks @seojangho. I've addressed your comment.

seojangho
seojangho approved these changes Jul 3, 2018
View reviewed changes
Copy link
Member

@seojangho seojangho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work.

@sanha
Copy link
Contributor

sanha commented Jul 3, 2018 •
edited

LGTM. I'm merging this.

@sanha sanha merged commit df44252 into apache:master Jul 3, 2018
@johnyangk johnyangk deleted the 54-fetch-fail branch July 5, 2018 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seojangho seojangho seojangho approved these changes

Assignees

@johnyangk johnyangk

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@johnyangk @sanha @seojangho