Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Dec 24, 2014

A variable shutdownCallback in SparkDeploySchedulerBackend can be accessed from multiple threads so it should be enclosed by synchronized block.

@SparkQA
Copy link

SparkQA commented Dec 24, 2014

Test build #24759 has finished for PR 3781 at commit 5942765.

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

Copy link
Member

Choose a reason for hiding this comment

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

On the one hand, this sounds like it could be an AtomicBoolean. On another hand -- this whole mechanism could be replaced by something more robust in java.util.concurrent

@sarutak sarutak changed the title [SPARK-4949] [SPARK-4949] shutdownCallback in SparkDeploySchedulerBackend should be enclosed by synchronized block. [SPARK-4949]shutdownCallback in SparkDeploySchedulerBackend should be enclosed by synchronized block. Dec 24, 2014
@SparkQA
Copy link

SparkQA commented Dec 24, 2014

Test build #24782 has finished for PR 3781 at commit 1b60fd1.

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

@SparkQA
Copy link

SparkQA commented Jan 8, 2015

Test build #25235 has finished for PR 3781 at commit 3e3631d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@sarutak
Copy link
Member Author

sarutak commented Jan 8, 2015

retest this please.

@SparkQA
Copy link

SparkQA commented Jan 8, 2015

Test build #25244 has finished for PR 3781 at commit 3e3631d.

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

@srowen
Copy link
Member

srowen commented Feb 16, 2015

This looks good to me. Most of it is making some fields private that look like they should be, simplifying one synchronization primitive, and does fix a theoretical concurrency problem. Tests pass. I will merge soon if there are no objections.

Copy link
Member

Choose a reason for hiding this comment

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

@sarutak would this not be simpler as a @volatile reference? that's how appId is handled below.

@sarutak
Copy link
Member Author

sarutak commented Feb 18, 2015

@srowen Thank you for picking up this PR. I've changed the shutdownCallback to be a simple volatile variable.

@SparkQA
Copy link

SparkQA commented Feb 18, 2015

Test build #27667 has finished for PR 3781 at commit 3c1c018.

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

@SparkQA
Copy link

SparkQA commented Feb 18, 2015

Test build #27668 has finished for PR 3781 at commit 42ca528.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class Partitioner(object):

Copy link
Member

Choose a reason for hiding this comment

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

OK, but why do you need this setter now?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's no longer needed so I remove it soon.

@SparkQA
Copy link

SparkQA commented Feb 18, 2015

Test build #27678 has finished for PR 3781 at commit c146c93.

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

@asfgit asfgit closed this in 82197ed Feb 18, 2015
@sarutak sarutak deleted the SPARK-4949 branch April 11, 2015 05:24
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