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.

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

[SPARK-18640] Add synchronization to TaskScheduler.runningTasksByExecutors #16073

Conversation

JoshRosen
Copy link
Contributor

What changes were proposed in this pull request?

The method TaskSchedulerImpl.runningTasksByExecutors() accesses the mutable executorIdToRunningTaskIds map without proper synchronization. In addition, as @markhamstra pointed out in #15986, the signature's use of parentheses is a little odd given that this is a pure getter method.

This patch fixes both issues.

How was this patch tested?

Covered by existing tests.

@SparkQA
Copy link

SparkQA commented Nov 30, 2016

Test build #69372 has finished for PR 16073 at commit c866a1a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@andrewor14
Copy link
Contributor

LGTM merging into master 2.1.

@andrewor14
Copy link
Contributor

and 2.0

asfgit pushed a commit that referenced this pull request Nov 30, 2016
…utors

## What changes were proposed in this pull request?

The method `TaskSchedulerImpl.runningTasksByExecutors()` accesses the mutable `executorIdToRunningTaskIds` map without proper synchronization. In addition, as markhamstra pointed out in #15986, the signature's use of parentheses is a little odd given that this is a pure getter method.

This patch fixes both issues.

## How was this patch tested?

Covered by existing tests.

Author: Josh Rosen <joshrosen@databricks.com>

Closes #16073 from JoshRosen/runningTasksByExecutors-thread-safety.

(cherry picked from commit c51c772)
Signed-off-by: Andrew Or <andrewor14@gmail.com>
@asfgit asfgit closed this in c51c772 Nov 30, 2016
asfgit pushed a commit that referenced this pull request Nov 30, 2016
…utors

## What changes were proposed in this pull request?

The method `TaskSchedulerImpl.runningTasksByExecutors()` accesses the mutable `executorIdToRunningTaskIds` map without proper synchronization. In addition, as markhamstra pointed out in #15986, the signature's use of parentheses is a little odd given that this is a pure getter method.

This patch fixes both issues.

## How was this patch tested?

Covered by existing tests.

Author: Josh Rosen <joshrosen@databricks.com>

Closes #16073 from JoshRosen/runningTasksByExecutors-thread-safety.

(cherry picked from commit c51c772)
Signed-off-by: Andrew Or <andrewor14@gmail.com>
@JoshRosen JoshRosen deleted the runningTasksByExecutors-thread-safety branch November 30, 2016 19:52
robert3005 pushed a commit to palantir/spark that referenced this pull request Dec 2, 2016
…utors

## What changes were proposed in this pull request?

The method `TaskSchedulerImpl.runningTasksByExecutors()` accesses the mutable `executorIdToRunningTaskIds` map without proper synchronization. In addition, as markhamstra pointed out in apache#15986, the signature's use of parentheses is a little odd given that this is a pure getter method.

This patch fixes both issues.

## How was this patch tested?

Covered by existing tests.

Author: Josh Rosen <joshrosen@databricks.com>

Closes apache#16073 from JoshRosen/runningTasksByExecutors-thread-safety.
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
…utors

## What changes were proposed in this pull request?

The method `TaskSchedulerImpl.runningTasksByExecutors()` accesses the mutable `executorIdToRunningTaskIds` map without proper synchronization. In addition, as markhamstra pointed out in apache#15986, the signature's use of parentheses is a little odd given that this is a pure getter method.

This patch fixes both issues.

## How was this patch tested?

Covered by existing tests.

Author: Josh Rosen <joshrosen@databricks.com>

Closes apache#16073 from JoshRosen/runningTasksByExecutors-thread-safety.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants