Skip to content
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-27071][CORE] Expose additional metrics in status.api.v1.StageData #24011

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
5 participants
@tomvanbussel
Copy link
Contributor

commented Mar 7, 2019

What changes were proposed in this pull request?

This PR exposes additional metrics in status.api.v1.StageData. These metrics were already computed for LiveStage, but they were never exposed to the user. This includes extra metrics about the JVM GC, executor (de)serialization times, shuffle reads and writes, and more.

How was this patch tested?

Existing tests.

cc @hvanhovell

@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@vanzin can you take a look?

@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

ok to test

@attilapiros

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

Unfortunately this change also increases the event log size meanwhile there are problems caused by the too huge event logs (think about a long running streaming app).

As this metrics are available via the monitoring subsystem and can be sent to an external system I have some doubts whether this is really needed.

@vanzin

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

@attilapiros I don't see any change that would affect the event log at all here?

(Haven't looked at everything in detail yet.)

@attilapiros

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@vanzin Oh I see SparkListenerStageCompleted already contains the TaskMetrics.

@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

Any more feedback here? Otherwise I am going to pull the trigger on this one.

@vanzin

This comment has been minimized.

Copy link
Contributor

commented Mar 21, 2019

I most probably won't get to reviewing this. But tests haven't run.

This is also technically an API breakage, because you're removing fields from the public REST types.

@tomvanbussel

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

If we cannot break the API, then I see two options:

  1. We leave the old fields, and we also add the TaskMetrics. This would mean that some fields are duplicated.
  2. We inline all the fields from TaskMetrics into StageData. This is not as nice (in my opinion), but there will be no duplicates.

Which option is better?

@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2019

@tomvanbussel lets go with option 2 for now.

@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented Apr 1, 2019

ok to test

@SparkQA

This comment has been minimized.

Copy link

commented Apr 1, 2019

Test build #104163 has finished for PR 24011 at commit 4974fa0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
@hvanhovell
Copy link
Contributor

left a comment

LGTM. I will defer to @vanzin for final sign-off.

@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

retest this please

@SparkQA

This comment has been minimized.

Copy link

commented May 15, 2019

Test build #105429 has finished for PR 24011 at commit 4974fa0.

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

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

retest this please

1 similar comment
@hvanhovell

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

retest this please

@SparkQA

This comment has been minimized.

Copy link

commented May 27, 2019

Test build #105829 has finished for PR 24011 at commit 4974fa0.

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

This comment has been minimized.

Copy link
Contributor

commented May 27, 2019

Merging to master. Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.