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

[improve][broker] Add topic_load_failed metric #19236

Merged
merged 7 commits into from
Jun 3, 2023

Conversation

tjiuming
Copy link
Contributor

@tjiuming tjiuming commented Jan 15, 2023

PIP: #18979

Motivation

Add topic_load_failed metric

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

If the box was checked, please highlight the changes

  • Dependencies (add or upgrade a dependency)
  • The public API
  • The schema
  • The default values of configurations
  • The threading model
  • The binary protocol
  • The REST endpoints
  • The admin CLI options
  • The metrics
  • Anything that affects deployment

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: tjiuming#20

@tjiuming
Copy link
Contributor Author

Tests to be completed

@github-actions github-actions bot added the doc-required Your PR changes impact docs and you will update later. label Jan 15, 2023
Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

please add a test for it

Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

please add a test for it

@sunheyi6
Copy link
Contributor

sunheyi6 commented Feb 15, 2023

@tjiuming You should associate issue with pr. Closes #18963 #18979 And some checks were not successful

@tjiuming tjiuming changed the title [improve][monitoring]PIP-231: Add topic_load_failed metric [improve]PIP-231: Add topic_load_failed metric Feb 15, 2023
@tjiuming
Copy link
Contributor Author

@tjiuming You should associate issue with pr. Closes #18963 #18979 And some checks were not successful

I'll close them after the PR merged

@sunheyi6
Copy link
Contributor

@tjiuming You should associate issue with pr. Closes #18963 #18979 And some checks were not successful

I'll close them after the PR merged

GitHub provides this function. After you associate the issue with pr, closing the pr will automatically close the issue.
refer to https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue

@github-actions
Copy link

github-actions bot commented Apr 2, 2023

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Apr 2, 2023
@Technoboy- Technoboy- added this to the 3.1.0 milestone Jun 1, 2023
@Technoboy- Technoboy- changed the title [improve]PIP-231: Add topic_load_failed metric [improve][metric] Add topic_load_failed metric Jun 1, 2023
@Technoboy- Technoboy- removed the Stale label Jun 1, 2023
@Technoboy- Technoboy- changed the title [improve][metric] Add topic_load_failed metric [improve][broker] Add topic_load_failed metric Jun 1, 2023
@codecov-commenter
Copy link

codecov-commenter commented Jun 2, 2023

Codecov Report

Merging #19236 (4d1fab7) into master (5e6e6ce) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #19236      +/-   ##
============================================
- Coverage     72.95%   72.92%   -0.04%     
+ Complexity    31914    31900      -14     
============================================
  Files          1867     1867              
  Lines        138485   138498      +13     
  Branches      15202    15202              
============================================
- Hits         101037   101003      -34     
- Misses        29438    29469      +31     
- Partials       8010     8026      +16     
Flag Coverage Δ
inttests 24.20% <85.71%> (+0.08%) ⬆️
systests 24.96% <42.85%> (-0.07%) ⬇️
unittests 72.22% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...rg/apache/pulsar/broker/service/BrokerService.java 81.17% <100.00%> (+0.51%) ⬆️
.../org/apache/pulsar/broker/service/PulsarStats.java 87.12% <100.00%> (+0.19%) ⬆️
.../pulsar/broker/stats/BrokerOperabilityMetrics.java 100.00% <100.00%> (ø)

... and 61 files with indirect coverage changes

@Technoboy-
Copy link
Contributor

/pulsarbot run-failure-checks

@Technoboy- Technoboy- merged commit 43b3622 into apache:master Jun 3, 2023
@lhotari
Copy link
Member

lhotari commented Jun 5, 2023

I wonder if this caused #20492 ?

@lhotari
Copy link
Member

lhotari commented Jun 5, 2023

The test added by this PR causes issues in CI. The test is disabled by #20494. There's a issue about rewriting the test in #20495 .

@Anonymitaet
Copy link
Member

@tjiuming this PR is labeled with doc-required. Does it need to add the new metric to https://pulsar.apache.org/docs/next/reference-metrics/#topic-metrics?

poorbarcode pushed a commit that referenced this pull request May 2, 2024
Co-authored-by: daojun <[email protected]>
(cherry picked from commit 43b3622)
poorbarcode pushed a commit that referenced this pull request May 2, 2024
Co-authored-by: daojun <[email protected]>
(cherry picked from commit 43b3622)
poorbarcode pushed a commit that referenced this pull request May 2, 2024
Co-authored-by: daojun <[email protected]>
(cherry picked from commit 43b3622)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request May 13, 2024
Co-authored-by: daojun <[email protected]>
(cherry picked from commit 43b3622)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request Jun 7, 2024
Co-authored-by: daojun <[email protected]>
(cherry picked from commit 43b3622)
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.

10 participants