-
Notifications
You must be signed in to change notification settings - Fork 28.3k
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
Conversation
@vanzin can you take a look? |
ok to test |
|
@attilapiros I don't see any change that would affect the event log at all here? (Haven't looked at everything in detail yet.) |
@vanzin Oh I see |
Any more feedback here? Otherwise I am going to pull the trigger on this one. |
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. |
If we cannot break the API, then I see two options:
Which option is better? |
@tomvanbussel lets go with option 2 for now. |
ok to test |
Test build #104163 has finished for PR 24011 at commit
|
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.
LGTM. I will defer to @vanzin for final sign-off.
retest this please |
Test build #105429 has finished for PR 24011 at commit
|
retest this please |
1 similar comment
retest this please |
Test build #105829 has finished for PR 24011 at commit
|
Merging to master. Thanks |
What changes were proposed in this pull request?
This PR exposes additional metrics in
status.api.v1.StageData
. These metrics were already computed forLiveStage
, 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