Skip to content

SPARK-7171: Added a method to retrieve metrics sources in TaskContext #5805

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
Closed
jacek-lewandowski wants to merge 1 commit into from
Closed

SPARK-7171: Added a method to retrieve metrics sources in TaskContext #5805

jacek-lewandowski wants to merge 1 commit into from

Conversation

@jacek-lewandowski
Copy link
Contributor

@jacek-lewandowski jacek-lewandowski commented Apr 30, 2015

No description provided.

@SparkQA
Copy link

SparkQA commented Apr 30, 2015

Test build #31406 has finished for PR 5805 at commit 92aa76f.

  • This patch fails MiMa tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch adds the following new dependencies:
    • jaxb-api-2.2.7.jar
    • jaxb-core-2.2.7.jar
    • jaxb-impl-2.2.7.jar
    • pmml-agent-1.1.15.jar
    • pmml-model-1.1.15.jar
    • pmml-schema-1.1.15.jar
  • This patch removes the following dependencies:
    • activation-1.1.jar
    • jaxb-api-2.2.2.jar
    • jaxb-impl-2.2.3-1.jar

@jacek-lewandowski
Copy link
Contributor Author

jacek-lewandowski commented Apr 30, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Apr 30, 2015

Test build #31412 has finished for PR 5805 at commit 9ed86ca.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
  • This patch does not change any dependencies.

squito
squito reviewed Apr 30, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/TaskContextImpl.scala Outdated
Copy link
Contributor

@squito squito Apr 30, 2015

Choose a reason for hiding this comment

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

will you get an NPE here if no metric system has been setup?

Copy link
Contributor

@squito squito Apr 30, 2015

Choose a reason for hiding this comment

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

nevermind, sorry -- I see that its never null, it can just be empty

@squito
Copy link
Contributor

squito commented Apr 30, 2015

Hi @jacek-lewandowski
I like this idea overall, but personally I am little hesitant to add something like this in the flurry before 1.4 is released since its changing public apis (even though its a non-breaking change). Not that I am against it if somebody else feels safe merging this -- just wanted to give you a heads up that you might need to sit on it a while.

@jacek-lewandowski
Copy link
Contributor Author

jacek-lewandowski commented May 4, 2015

@squito so who should I ask for this?

andrewor14
andrewor14 reviewed Jun 18, 2015
View reviewed changes
core/src/main/scala/org/apache/spark/TaskContext.scala Outdated
Copy link
Contributor

@andrewor14 andrewor14 Jun 18, 2015

Choose a reason for hiding this comment

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

This special tag is only necessary if there is a java doc (please add one). The javadoc here will be incorrectly displayed on the docs page. For more info see other places where :: DeveloperApi :: is used.

@andrewor14
Copy link
Contributor

andrewor14 commented Jun 18, 2015

@jacek-lewandowski once you rebase to master I'll take a look at this in more detail.

@jacek-lewandowski
Copy link
Contributor Author

jacek-lewandowski commented Jun 22, 2015

@andrewor14 rebased

@SparkQA
Copy link

SparkQA commented Jun 22, 2015

Test build #35435 has finished for PR 5805 at commit 3d06806.

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

@SparkQA
Copy link

SparkQA commented Jun 29, 2015

Test build #36031 has finished for PR 5805 at commit 3d06806.

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

@andrewor14
Copy link
Contributor

andrewor14 commented Jul 17, 2015

@jacek-lewandowski I just looked at this and the changes look fine. Unfortunately it doesn't merge with master again. Could you rebase this once more? Also, I'd like to see my documentation comment addressed. Once you've done these things then we can probably merge this.

@SparkQA
Copy link

SparkQA commented Jul 20, 2015

Test build #37826 has finished for PR 5805 at commit d685e53.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • class ShuffledRowRDD(

@SparkQA
Copy link

SparkQA commented Jul 20, 2015

Test build #37844 has finished for PR 5805 at commit 9837a67.

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

@SparkQA
Copy link

SparkQA commented Jul 20, 2015

Test build #37852 has finished for PR 5805 at commit ed20bda.

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

@jacek-lewandowski
Copy link
Contributor Author

jacek-lewandowski commented Jul 20, 2015

@andrewor14 is it ok now?

@andrewor14
Copy link
Contributor

andrewor14 commented Jul 21, 2015

LGTM, merging into master. Thanks @jacek-lewandowski.

@asfgit asfgit closed this in 3195491 Jul 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jacek-lewandowski @SparkQA @squito @andrewor14