Skip to content

Conversation

@fsamuel-bs
Copy link

@fsamuel-bs fsamuel-bs commented Oct 7, 2020

Upstream SPARK-33088 ticket and apache#29977

Done.

What changes were proposed in this pull request?

Add a set of endpoints on ExecutorPlugin which are called on the lifecycle of a Task. Main usage of such endpoints is for tracing purposes, i.e. to propagate the inbound tracing information from TaskContext#getLocalProperties to a tracing framework of choice (in our case, https://github.com/palantir/tracing-java/).

How was this patch tested?

Unit tests on ExecutorPluginTaskSuite.

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

@fsamuel-bs fsamuel-bs changed the title Add Add ExecutorPlugin methods which track the lifecycle of a Task Oct 7, 2020
@fsamuel-bs fsamuel-bs changed the title Add ExecutorPlugin methods which track the lifecycle of a Task Add ExecutorPlugin methods called on Task Start/End Oct 7, 2020
Comment on lines +34 to +37
// Static value modified by testing plugins to ensure plugins are called correctly.
public static int numOnTaskStart = 0;
public static int numOnTaskSucceeded = 0;
public static int numOnTaskFailed = 0;
Copy link

Choose a reason for hiding this comment

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

Apologies, not following. Why is non-static insufficient to ensure that?

Copy link
Author

Choose a reason for hiding this comment

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

We could, I was following the pattern of https://github.com/palantir/spark/blob/master/core/src/test/java/org/apache/spark/ExecutorPluginSuite.java#L35. I think this is a preference thing, and I particularly would rather have these be variables in each plugin. Will refactor such this is the case.

Copy link
Author

Choose a reason for hiding this comment

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

Actually, we need a static variable because we don't have access to the instances of the plugin being instantiated here (plugins are instantiated on the spark executors). The way upstream does now is a bit more verbose, but still relies on static variables... this is a bit simpler, so will leave like this.

Copy link

@rshkv rshkv left a comment

Choose a reason for hiding this comment

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

Very cool! I left some minor comments but can merge as-is.

@fsamuel-bs fsamuel-bs force-pushed the ssouza/onTask-executorPlugin branch from a19df5e to 42d60c2 Compare October 8, 2020 17:07
@rshkv rshkv merged commit 73ce892 into master Oct 8, 2020
@rshkv rshkv deleted the ssouza/onTask-executorPlugin branch October 8, 2020 18:27
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