Skip to content

[monitoring][broker][fix] Fix EntryFilter stats - #17605

Merged
nicoloboschi merged 4 commits into
apache:masterfrom
tjiuming:fix/entry_filter_stats_fix
Sep 21, 2022
Merged

[monitoring][broker][fix] Fix EntryFilter stats#17605
nicoloboschi merged 4 commits into
apache:masterfrom
tjiuming:fix/entry_filter_stats_fix

Conversation

@tjiuming

@tjiuming tjiuming commented Sep 13, 2022

Copy link
Copy Markdown
Contributor

Fixes #17595

Motivation

Fix #17595 when no EntryFilters configured

Documentation

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

@tjiuming tjiuming mentioned this pull request Sep 13, 2022
Closed
2 tasks
@tjiuming

tjiuming commented Sep 13, 2022

Copy link
Copy Markdown
Contributor Author

@michaeljmarshall @codelipenghui PTAL

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Sep 13, 2022
gaoran10
gaoran10 reviewed Sep 13, 2022
View reviewed changes

@gaoran10 gaoran10 left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, but it seems that there is a potential problem, if there are some markers, the filterProcessCount will be not equal with rejectedCount + rescheduledCount + acceptCount, which may cause misunderstanding.

@tjiuming

tjiuming commented Sep 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

if there are some markers, the filterProcessCount will be not equal with rejectedCount + rescheduledCount + acceptCount, which may cause misunderstanding.

@gaoran10 could you please describe more clearly?

gaoran10
gaoran10 approved these changes Sep 13, 2022
View reviewed changes

@gaoran10 gaoran10 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

nicoloboschi
nicoloboschi approved these changes Sep 13, 2022
View reviewed changes

@nicoloboschi nicoloboschi left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM
just a nit for avoiding to call Collections.isEmpty multiple times

Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/AbstractBaseDispatcher.java Outdated
@tjiuming

tjiuming commented Sep 14, 2022

Copy link
Copy Markdown
Contributor Author

/pulsarbot rerun-failure-checks

@nicoloboschi
nicoloboschi merged commit 8441f67 into apache:master Sep 21, 2022
@nicoloboschi nicoloboschi added this to the 2.12.0 milestone Sep 21, 2022
Technoboy- pushed a commit that referenced this pull request Sep 26, 2022
@tjiuming @Technoboy-
[monitoring][broker][fix] Fix EntryFilter stats (#17605)
bc0acb9 
* fix entryFilter stats

* fix test

* add test comment

* review fix
@Technoboy- Technoboy- modified the milestones: 2.12.0, 2.11.0 Sep 26, 2022
@tjiuming
tjiuming deleted the fix/entry_filter_stats_fix branch November 1, 2022 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gaoran10 gaoran10 gaoran10 approved these changes
@nicoloboschi nicoloboschi nicoloboschi approved these changes

Assignees

@tjiuming tjiuming

Labels

area/metrics cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs

Projects

None yet

Milestone

2.11.0

Development

Successfully merging this pull request may close these issues.

[Bug] Entry filters stats should not be increased if there's no entry filter

5 participants

@tjiuming @gaoran10 @nicoloboschi @Technoboy- @dao-jun