[improve][broker] Decouple pulsar_storage_backlog_age_seconds metric with backlogQuota check #23619
+221
−120
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shibd
added
type/refactor
shibd
requested review from
lhotari,
asafm,
Jason918,
codelipenghui,
poorbarcode and
michaeljmarshall
github-actions
bot
added
doc-label-missing
doc-not-needed
…from backlogQuota policy
shibd
force-pushed
the
sperate_metrics
branch
from
8b8ffc8
to
e2a102f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23619 +/- ##
============================================
+ Coverage 73.57% 74.42% +0.85%
- Complexity 32624 35003 +2379
============================================
Files 1877 1944 +67
Lines 139502 147136 +7634
Branches 15299 16224 +925
============================================
+ Hits 102638 109512 +6874
- Misses 28908 29184 +276
- Partials 7956 8440 +484
Flags with carried forward coverage won't be shown. Click here to find out more.
|
shibd
changed the title
poorbarcode
reviewed
Nov 27, 2024
| } | ||
| return CompletableFuture.completedFuture(false); | ||
| return CompletableFuture.completedFuture(null); |
There was a problem hiding this comment.
I am considering whether the value of the metric will be fine in the following case:
- ledgers:
[1:0~1:99, 2:0~2:99] - create a cursor
s1at1:0 - the value of
pulsar_storage_backlog_age_secondsrelates to1:0 - unsubscribe
s1 - the value will be never updated anymore, in other words, it always relates to
1:0
|
|
||
| boolean expired = MessageImpl.isEntryExpired(backlogQuotaLimitInSecond, entryTimestamp); | ||
| if (log.isDebugEnabled()) { | ||
| log.debug("[{}] Time based backlog quota check. Oldest unacked entry read from BK. " |
| if (oldestPositionInfo == null) { | ||
| return CompletableFuture.completedFuture(false); | ||
| } | ||
| if (!hasBacklogs(brokerService.pulsar().getConfiguration().isPreciseTimeBasedBacklogQuotaCheck())) { |
There was a problem hiding this comment.
Since we have checked it in the method checkTimeBacklogExceeded [see pic below], can we skip to check it again here?
| } | ||
| CompletableFuture<Boolean> future = new CompletableFuture<>(); | ||
| CompletableFuture<Void> future = new CompletableFuture<>(); | ||
| // Check if first unconsumed message(first message after mark delete position) | ||
| // for slowest cursor's has expired. | ||
| Position position = ledger.getNextValidPosition(oldestMarkDeletePosition); |
There was a problem hiding this comment.
Will this position be x:-1? If yes, it may cause an EntryNotExists error. Could you add a test for this scenario?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
#21709 introduced a valuable metric: pulsar_storage_backlog_age_seconds, but it only updates when a topic has the message_age backlog quota policy.
Modifications
backlogQuotaCheckeris also used to periodically update thepulsar_storage_backlog_age_secondsmetric, regardless of whether the topic has a backlog quota policy.Verifying this change
backlogsAgeMetricsNoPreciseWithoutBacklogQuotaandbacklogsAgeMetricsPreciseWithoutBacklogQuotato cover this change.Documentation
docdoc-requireddoc-not-neededdoc-completeMatching PR in forked repository
PR in forked repository: