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.

Sign up for GitHub

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

Jump to bottom

[FLINK-14303][metrics] Replace JobManagerMetricGroup with ResourceManagerMetricGroup in ResourceManager #9827

Closed
wants to merge 7 commits into from
Closed

[FLINK-14303][metrics] Replace JobManagerMetricGroup with ResourceManagerMetricGroup in ResourceManager #9827

wants to merge 7 commits into from

Conversation

tillrohrmann
Copy link
Contributor

What is the purpose of the change

This PR is based on #9825.

This PR introduces the ResourceManagerMetricGroup and replaces the JobManagerMetricGroup with it in the ResourceManager.

cc @zentol.

Verifying this change

  • Verified manually using the JMXMetricReporter that all metrics are still reported.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
  • The serializers: (no)
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Yarn/Mesos, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)
  • If yes, how is the feature documented? (not applicable)

@flinkbot
Copy link
Collaborator

flinkbot commented Oct 1, 2019 •
edited

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 53201b4 (Wed Oct 16 08:34:59 UTC 2019)

Warnings:

  • No documentation files were touched! Remember to keep the Flink docs up to date!

Mention the bot in a comment to re-run the automated checks.

Review Progress

  • ❓ 1. The [description] looks good.
  • ❓ 2. There is [consensus] that the contribution should go into to Flink.
  • ❓ 3. Needs [attention] from.
  • ❓ 4. The change fits into the overall [architecture].
  • ❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.


The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commands
The @flinkbot bot supports the following commands:

  • @flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
  • @flinkbot approve all to approve all aspects
  • @flinkbot approve-until architecture to approve everything until architecture
  • @flinkbot attention @username1 [@username2 ..] to require somebody's attention
  • @flinkbot disapprove architecture to remove an approval you gave earlier

@tillrohrmann tillrohrmann mentioned this pull request Oct 1, 2019
Closed
@flinkbot
Copy link
Collaborator

flinkbot commented Oct 1, 2019 •
edited

CI report:

@tillrohrmann tillrohrmann mentioned this pull request Oct 1, 2019
Closed
zentol
zentol approved these changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@zentol zentol left a comment

Choose a reason for hiding this comment

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

+1 (excluding commits from previous PRs)

Has a merge conflict though.

@zentol zentol self-assigned this Oct 2, 2019
@tillrohrmann
[hotfix] Reduce code duplication in MetricUtils
eb7e502
@tillrohrmann
[FLINK-14299] Introduce ProcessMetricGroup
5e3a490 
The ProcessMetricGroup encapsulates the Status and system metrics of the ClusterEntrypoint.
These have been factored out of the JobManagerMetricGroup. In order to maintain backwards
compatibility, the same scope as the JobManagerMetricGroup is being used.
@tillrohrmann
fixup! [FLINK-14299] Introduce ProcessMetricGroup
9fa1089
@tillrohrmann
[hotfix] Factor out AbstractImitatingJobManagerMetricGroup super class
b61bcef 
The AbstractImitatingJobManagerMetricGroup imitates the reporting of the
JobManagerMetricGroup and can be used for metrics which are factored out
of the JobManagerMetricGroup without breaking backwards compatibility wrt
the reported metrics.
@tillrohrmann
[FLINK-14303] Introduce ResourceManagerMetricGroup
495f804 
Introduce a new metric group called ResourceManagerMetricGroup which can be used
to report ResourceManager specific metrics.
@tillrohrmann
[hotfix] Remove unused MetricRegistry from ResourceManager
f5aa4e9
@tillrohrmann
[FLINK-14303][metrics] Replace JobManagerMetricGroup with ResourceMan…
53201b4 
…agerMetricGroup in ResourceManager

With this commit, the ResourceManager uses the ResourceManagerMetricGroup to register its
metrics.
@tillrohrmann
Copy link
Contributor Author

Thanks for the review @zentol. I've rebased and force pushed to let CI run another time.

@tillrohrmann
Copy link
Contributor Author

Merged.

@tillrohrmann tillrohrmann deleted the FLINK-14303 branch October 2, 2019 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zentol zentol zentol approved these changes

Assignees

@zentol zentol

Labels
component=Runtime/Coordination component=Runtime/Metrics review=description?
Projects
None yet
Milestone
No milestone
4 participants
@tillrohrmann @flinkbot @zentol @rmetzger