-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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-7171: Added a method to retrieve metrics sources in TaskContext #5805
Conversation
Test build #31406 has finished for PR 5805 at commit
|
92aa76f
to
9ed86ca
Compare
retest this please |
Test build #31412 has finished for PR 5805 at commit
|
@@ -94,5 +97,8 @@ private[spark] class TaskContextImpl( | |||
override def isRunningLocally(): Boolean = runningLocally | |||
|
|||
override def isInterrupted(): Boolean = interrupted | |||
|
|||
override def getMetricsSources(sourceName: String): Seq[Source] = | |||
metricsSystem.getSourcesByName(sourceName) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
Hi @jacek-lewandowski |
@squito so who should I ask for this? |
@@ -135,6 +136,10 @@ abstract class TaskContext extends Serializable { | |||
@DeveloperApi | |||
def taskMetrics(): TaskMetrics | |||
|
|||
/** ::DeveloperApi:: */ |
There was a problem hiding this comment.
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.
@jacek-lewandowski once you rebase to master I'll take a look at this in more detail. |
9ed86ca
to
3d06806
Compare
@andrewor14 rebased |
Test build #35435 has finished for PR 5805 at commit
|
Test build #36031 has finished for PR 5805 at commit
|
@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. |
3d06806
to
d685e53
Compare
Test build #37826 has finished for PR 5805 at commit
|
d685e53
to
9837a67
Compare
Test build #37844 has finished for PR 5805 at commit
|
9837a67
to
ed20bda
Compare
Test build #37852 has finished for PR 5805 at commit
|
@andrewor14 is it ok now? |
LGTM, merging into master. Thanks @jacek-lewandowski. |
No description provided.