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] allow client skips the non-continued deleted ranges stats #18263

Closed
wants to merge 3 commits into from

Conversation

leizhiyuan
Copy link
Contributor

@leizhiyuan leizhiyuan commented Oct 31, 2022

Fixes #18240

Master Issue: #18240

Motivation

Modifications

add a flag getTotalNonContiguousDeletedMessagesRange to allow users can skip the non-continued deleted ranges stats

Verifying this change

  • Make sure that the change passes the CI checks.

leizhiyuan#5

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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
  • Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository:

@leizhiyuan
Copy link
Contributor Author

maybe we should use GetStatsOptions next pr, not original name like getPreciseBacklog,subscriptionBacklogSize etc.

@github-actions
Copy link

@leizhiyuan Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

@codelipenghui codelipenghui added this to the 2.12.0 milestone Oct 31, 2022
@github-actions github-actions bot added doc-not-needed Your PR changes do not impact docs and removed doc-label-missing labels Oct 31, 2022
@leizhiyuan leizhiyuan changed the title feat:allow client skips the non-continued deleted ranges stats [improve][broker] allow client skips the non-continued deleted ranges stats Oct 31, 2022
@labuladong
Copy link
Contributor

There is a failed broker test in your leizhiyuan#4 :
https://github.com/leizhiyuan/pulsar/actions/runs/3373856231/jobs/5598944551#step:10:1209

if it is caused by your change please fix it.

@leizhiyuan
Copy link
Contributor Author

There is a failed broker test in your leizhiyuan#4 : https://github.com/leizhiyuan/pulsar/actions/runs/3373856231/jobs/5598944551#step:10:1209

if it is caused by your change please fix it.

leizhiyuan#5

I didn't sync the master branch ,now I opened a new pr

@leizhiyuan
Copy link
Contributor Author

There is a failed broker test in your leizhiyuan#4 : https://github.com/leizhiyuan/pulsar/actions/runs/3373856231/jobs/5598944551#step:10:1209
if it is caused by your change please fix it.

leizhiyuan#5

I didn't sync the master branch ,now I opened a new pr

image

Add some case seems be flaky, It is not related with the pr, I ran ok in my machine。

@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2022

Codecov Report

Merging #18263 (8460be2) into master (46c0438) will increase coverage by 5.87%.
The diff coverage is 83.01%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #18263      +/-   ##
============================================
+ Coverage     38.84%   44.71%   +5.87%     
- Complexity     8286    10591    +2305     
============================================
  Files           683      748      +65     
  Lines         67323    73915    +6592     
  Branches       7215     8167     +952     
============================================
+ Hits          26152    33052    +6900     
+ Misses        38210    37169    -1041     
- Partials       2961     3694     +733     
Flag Coverage Δ
unittests 44.71% <83.01%> (+5.87%) ⬆️

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

Impacted Files Coverage Δ
...he/pulsar/broker/admin/v2/NonPersistentTopics.java 62.03% <ø> (ø)
...rg/apache/pulsar/broker/service/BrokerService.java 57.11% <0.00%> (-0.81%) ⬇️
...n/java/org/apache/pulsar/broker/service/Topic.java 18.18% <ø> (ø)
...ker/stats/prometheus/NamespaceStatsAggregator.java 0.00% <0.00%> (ø)
...pulsar/broker/admin/impl/PersistentTopicsBase.java 59.53% <81.81%> (+6.96%) ⬆️
...pache/pulsar/broker/admin/v1/PersistentTopics.java 46.49% <100.00%> (+0.79%) ⬆️
...pache/pulsar/broker/admin/v2/PersistentTopics.java 75.54% <100.00%> (+4.82%) ⬆️
...sar/broker/resourcegroup/ResourceGroupService.java 38.10% <100.00%> (+0.33%) ⬆️
...oker/service/nonpersistent/NonPersistentTopic.java 57.46% <100.00%> (+2.57%) ⬆️
...ker/service/persistent/PersistentSubscription.java 68.32% <100.00%> (+3.90%) ⬆️
... and 304 more

Copy link
Member

@nodece nodece left a comment

Choose a reason for hiding this comment

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

LGTM, I left some comments. Please help to confirm.

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Dec 22, 2022
@congbobo184
Copy link
Contributor

congbobo184 commented Dec 24, 2022

@leizhiyuan could you please merge the apache/master, then rerun the test

@github-actions github-actions bot removed the Stale label Dec 25, 2022
@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Jan 24, 2023
@poorbarcode
Copy link
Contributor

@leizhiyuan could you please rebase master and take a look at the failed tests?

@poorbarcode poorbarcode modified the milestones: 3.0.0, 3.1.0 Apr 10, 2023
@github-actions github-actions bot removed the Stale label Apr 11, 2023
@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label May 11, 2023
@Technoboy- Technoboy- modified the milestones: 3.1.0, 3.2.0 Jul 31, 2023
@Technoboy- Technoboy- modified the milestones: 3.2.0, 3.3.0 Dec 22, 2023
@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@lhotari
Copy link
Member

lhotari commented Oct 14, 2024

please rebase.

@lhotari
Copy link
Member

lhotari commented Oct 14, 2024

related to #23380

@leizhiyuan
Copy link
Contributor Author

It's been too long (2 years) since the pull request. I can't even remember some of the code at that time. I will close this PR.

@leizhiyuan leizhiyuan closed this Oct 14, 2024
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.

[improve][broker] ResourceGroupService alloc much memory even we do not use