-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[feat][broker] Implementation of PIP-323: Complete Backlog Quota Telemetry #21816
Conversation
@asafm Please add the following content to your PR description and select a checkbox:
|
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Impressive work @asafm!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, noted a couple of ideas below.
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
...-broker/src/test/java/org/apache/pulsar/broker/stats/prometheus/PrometheusMetricsClient.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BacklogQuotaManagerTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BacklogQuotaManagerTest.java
Outdated
Show resolved
Hide resolved
Thanks for the comments, @dragosvictor, very helpful! I replied to all. Can you re-review and resolve if it is solved? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for following up @asafm, looks good to me!
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #21816 +/- ##
============================================
- Coverage 73.69% 73.59% -0.11%
+ Complexity 32352 32343 -9
============================================
Files 1858 1860 +2
Lines 138144 138396 +252
Branches 15141 15175 +34
============================================
+ Hits 101812 101855 +43
- Misses 28490 28646 +156
- Partials 7842 7895 +53
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will continue to review the remaining 2 classes.
- PersistentTopic.java
- BacklogQuotaManagerTest.java
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorContainer.java
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BacklogQuotaManagerTest.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BacklogQuotaManagerTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BacklogQuotaManagerTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, which test covers the cached timeBasedBacklogQuotaCheckResult
really works?
@dragosvictor Can I resolve your comments? |
@asafm Yes, for some reason I don't have access to resolve my own comments. |
@codelipenghui This is a large change and causes a lot of merge conflicts. Do we really want this in maintenance branches? |
@lhotari Yes, so I choose to create PR to cherry-pick it. So that I can get more eyes for review. The reason is the feature is existing for long time, but users don't have way to move it production due to the missed metrics(alerts). |
…metry (apache#21816) (cherry picked from commit 59782b3)
…metry (apache#21816) (apache#22740) Co-authored-by: Asaf Mesika <asaf.mesika@gmail.com> (cherry picked from commit ce7a07b)
…metry (apache#21816) (apache#22740) Co-authored-by: Asaf Mesika <asaf.mesika@gmail.com> (cherry picked from commit ce7a07b)
…metry (apache#21816) (apache#22740) Co-authored-by: Asaf Mesika <asaf.mesika@gmail.com> (cherry picked from commit ce7a07b)
@codelipenghui Do you also have a chance to cherry-pick this to branch-3.2 ? there are quite a few merge conflicts. |
…metry (apache#21816) (apache#22740) Co-authored-by: Asaf Mesika <asaf.mesika@gmail.com> (cherry picked from commit ce7a07b)
PIP: #21709
Motivation
As explained in the PIP, the motivation is to allow users of Pulsar to set an alert when the actual age of the oldest unacknowledged message for a topic is nearing its defined backlog quota time limit.
Modifications
TopicStats
per the PIPPrometheusMetricsClient
was introduced, making retrieving and querying Prometheus metrics easier. The primary parsing method was refactored out ofPrometheusMetricsTest
as at least 7 test classes used this parse method directly, which didn't seem appropriate.Consumer.seek()
was enhanced as it lacked an important distinction to the effect of it. Added test to verify.log4j.xml
topulsar-broker/src/test/resources
, making it easy for developers to change the log level when running unit tests. Before they copy-paste a file into it and delete it.Verifying this change
Does this pull request potentially affect one of the following parts:
If the box was checked, please highlight the changes
Documentation
doc
doc-required
doc-not-needed
doc-complete
Matching PR in forked repository
PR in forked repository: asafm#4