Skip to content

Conversation

@staple
Copy link
Contributor

@staple staple commented Sep 14, 2014

Also made some cosmetic cleanups.

@SparkQA
Copy link

SparkQA commented Sep 14, 2014

Can one of the admins verify this patch?

@JoshRosen
Copy link
Contributor

Jenkins, this is ok to test.

@SparkQA
Copy link

SparkQA commented Sep 14, 2014

QA tests have started for PR 2385 at commit 10ba6e1.

  • This patch merges cleanly.

@jyotiska
Copy link
Contributor

LGTM.

@SparkQA
Copy link

SparkQA commented Sep 14, 2014

QA tests have finished for PR 2385 at commit 10ba6e1.

  • This patch fails unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class JavaStackTrace(object):

@staple
Copy link
Contributor Author

staple commented Sep 14, 2014

Hi, the above failure in NetworkReceiverSuite.scala seems like it may be unrelated to this patch. That test also passed when I ran locally.

@SparkQA
Copy link

SparkQA commented Sep 14, 2014

QA tests have started for PR 2385 at commit 10ba6e1.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have finished for PR 2385 at commit 10ba6e1.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

it's better to only call extract_concise_traceback() once, such as:

self._callsite = extract_concise_traceback()
if self._callsite is None:
   xxxx

@davies
Copy link
Contributor

davies commented Sep 15, 2014

LGTM, just one minor comment, it's not must to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like I also need to put JavaStackTrace here instead of SparkContext.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are just internal interfaces, so it's fine to not have all here. If having, it should be JavaStackTrace

@staple
Copy link
Contributor Author

staple commented Sep 15, 2014

Hi - I addressed the review comments and made some additional cosmetic changes.

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have started for PR 2385 at commit 7b3bb13.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Sep 15, 2014

QA tests have finished for PR 2385 at commit 7b3bb13.

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

@JoshRosen
Copy link
Contributor

This looks good to me, so I'm going to merge it. Thanks!

@asfgit asfgit closed this in 60050f4 Sep 16, 2014
@staple
Copy link
Contributor Author

staple commented Sep 16, 2014

Great! Thanks to all the reviewers.

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.

5 participants