Skip to content

Conversation

@LucaCanali
Copy link
Contributor

@LucaCanali LucaCanali commented Jul 23, 2020

What changes were proposed in this pull request?

Document the dependency between the config spark.metrics.staticSources.enabled and JVMSource registration.

Why are the changes needed?

This PT just documents the dependency between config spark.metrics.staticSources.enabled and JVM source registration.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manually tested.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@dongjoon-hyun
Copy link
Member

Hi, SPARK-32409 is this issue which is not merged yet, @LucaCanali .

The implementation of spark.metrics.staticSources.enabled in SPARK-32409 introduces a bug:

Copy link
Member

Choose a reason for hiding this comment

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

This is a partial revert of another your previous commit, #22279, two year ago, isn't it?

[SPARK-25277][YARN] YARN applicationMaster metrics should not register static metrics

Copy link
Member

Choose a reason for hiding this comment

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

What happens in YARN applicationMaster after this PR?

@LucaCanali
Copy link
Contributor Author

@dongjoon-hyun thank you very much for your quick reply on this and the comments.
My bad, I had forgotten about #22279. There are solid reasons to skip JVM Source registration for YARN Application Master case, as discussed there.

I can see a couple of options to move this forward:

  • One alternative is to add an additional flag as an argument to the start method, so that JVMClass registration can be done in the general case, but also be skipped for the YARN AM case, as in the WIP code I just pushed.
  • Alternatively, we can consider the current behavior (dependency of the config spark.metrics.register.static.sources.enabled and JVM Class registration) as acceptable and simply refocus this into a documentation PR, that is documenting the behavior in the monitoring doc.

Any thoughts?

@dongjoon-hyun
Copy link
Member

I prefer the second approach, @LucaCanali .

@LucaCanali LucaCanali force-pushed the bugJVMMetricsRegistration branch from a485e28 to 8988173 Compare August 10, 2020 08:25
@LucaCanali LucaCanali changed the title [SPARK-32409][CORE] Remove dependency between spark.metrics.staticSources.enabled and JVMSource registration [SPARK-32409][DOC] Document dependency between spark.metrics.staticSources.enabled and JVMSource registration Aug 10, 2020
@LucaCanali
Copy link
Contributor Author

I prefer the second approach, @LucaCanali .

OK for me, @dongjoon-hyun thanks for looking at it.
I have updated the PR to cover the documentation only. While doing that I also some minor typos in related entries in the documentation. Is it OK to piggyback those changes here or do you prefer a separate PR?

dongjoon-hyun pushed a commit that referenced this pull request Aug 10, 2020
…urces.enabled and JVMSource registration

### What changes were proposed in this pull request?
Document the dependency between the config `spark.metrics.staticSources.enabled` and JVMSource registration.

### Why are the changes needed?

This PT just documents the dependency between config `spark.metrics.staticSources.enabled` and JVM source registration.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?
Manually tested.

Closes #29203 from LucaCanali/bugJVMMetricsRegistration.

Authored-by: Luca Canali <luca.canali@cern.ch>
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
(cherry picked from commit 99f50c6)
Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
@dongjoon-hyun
Copy link
Member

Thank you, @LucaCanali . Merged to master/3.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants