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-3458] enable python "with" statements for SparkContext #2335

Closed
wants to merge 1 commit into from

Conversation

mattf
Copy link
Contributor

@mattf mattf commented Sep 9, 2014

allow for best practice code,

try:
  sc = SparkContext()
  app(sc)
finally:
  sc.stop()

to be written using a "with" statement,

with SparkContext() as sc:
  app(sc)

allow for best practice code,

try:
  sc = SparkContext()
  app(sc)
finally:
  sc.stop()

to be written using a "with" statement,

with SparkContext() as sc:
  app(sc)
@mattf
Copy link
Contributor Author

mattf commented Sep 9, 2014

@davies i'd appreciate your input on this

@SparkQA
Copy link

SparkQA commented Sep 9, 2014

QA tests have started for PR 2335 at commit 5b4e37c.

  • This patch merges cleanly.

sc = SparkContext()
self.assertNotEqual(SparkContext._active_spark_context, None)
sc.stop()
self.assertEqual(SparkContext._active_spark_context, None)
Copy link
Contributor

Choose a reason for hiding this comment

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

self.assertIsNone(xxx) is better than self.assertEqual(xxx, None)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

accordning to https://docs.python.org/2/library/unittest.html#assert-methods assertIsNone and assertIsNotNone are new to python 2.7 and my (potentially incorrect) understanding is we want to target 2.6.

thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I missed that, we definitely need to target Python2.6.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

side note: i can't even find a box w/ py2.6 (it's so old!)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I know :( . Unfortunately, Python 2.6 is still the default Python version on a lot of Linux distributions, so we've been trying to maintain backwards-compatibility in order to ensure a good out-of-the-box experience for those users.

@davies
Copy link
Contributor

davies commented Sep 9, 2014

@mattf It's cool to have this, just one minor comment about tests.

@SparkQA
Copy link

SparkQA commented Sep 9, 2014

QA tests have finished for PR 2335 at commit 5b4e37c.

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

@mattf
Copy link
Contributor Author

mattf commented Sep 9, 2014

This patch fails unit tests.

failures are unrelated to this patch

@davies
Copy link
Contributor

davies commented Sep 9, 2014

Jenkins, retest this please.

@davies
Copy link
Contributor

davies commented Sep 9, 2014

I'm OK to merge this.

@SparkQA
Copy link

SparkQA commented Sep 9, 2014

QA tests have started for PR 2335 at commit 5b4e37c.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 9, 2014

QA tests have finished for PR 2335 at commit 5b4e37c.

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

@mattf
Copy link
Contributor Author

mattf commented Sep 9, 2014

This patch fails unit tests.

still not this patch's fault

@rxin
Copy link
Contributor

rxin commented Sep 9, 2014

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 9, 2014

QA tests have started for PR 2335 at commit 5b4e37c.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 9, 2014

QA tests have finished for PR 2335 at commit 5b4e37c.

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

@rxin
Copy link
Contributor

rxin commented Sep 10, 2014

Merging in master. Thanks!

@asfgit asfgit closed this in 25b5b86 Sep 10, 2014
@mattf mattf deleted the SPARK-3458 branch September 10, 2014 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants