Skip to content

Conversation

@MrBago
Copy link
Contributor

@MrBago MrBago commented Jul 17, 2018

What changes were proposed in this pull request?

Followup for #21719.
Update spark.ml training code to fully wrap instrumented methods and remove old instrumentation APIs.

How was this patch tested?

existing tests.

Please review http://spark.apache.org/contributing.html before opening a pull request.

@SparkQA
Copy link

SparkQA commented Jul 17, 2018

Test build #93207 has finished for PR 21799 at commit e43491c.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jul 18, 2018

Test build #93238 has finished for PR 21799 at commit 5ce5dd1.

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

@MrBago MrBago changed the title [SPARK-24747][ML] Update spark.ml to use Instrumentation.instrumented. [SPARK-24852][ML] Update spark.ml to use Instrumentation.instrumented. Jul 18, 2018
Copy link
Member

@jkbradley jkbradley left a comment

Choose a reason for hiding this comment

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

This LGTM except for 1 suggestion:
We could make the Instrumentation constructor private like this:

private[spark] class Instrumentation private () extends Logging {

That would force developers to go through the instrumented code path.

@SparkQA
Copy link

SparkQA commented Jul 19, 2018

Test build #93254 has finished for PR 21799 at commit dddccf6.

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

@jkbradley
Copy link
Member

LGTM
Merging with master
Thanks @MrBago !

@asfgit asfgit closed this in 3cb1b57 Jul 20, 2018
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