-
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
[improve][broker] Replaced checkBackloggedCursors with checkBackloggedCursor(single subscription check) upon subscription #19343
[improve][broker] Replaced checkBackloggedCursors with checkBackloggedCursor(single subscription check) upon subscription #19343
Conversation
Codecov Report
@@ Coverage Diff @@
## master #19343 +/- ##
=============================================
+ Coverage 32.42% 62.50% +30.08%
- Complexity 6347 25420 +19073
=============================================
Files 1644 1818 +174
Lines 123712 133112 +9400
Branches 13486 14644 +1158
=============================================
+ Hits 40109 83205 +43096
+ Misses 77694 42231 -35463
- Partials 5909 7676 +1767
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Question: |
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 think it would be better to call it only for the single subscription, instead of for all of them
Also, If we still want to call this operation, checkBackloggedCursors asynchronously but more frequently, we could run such async operations in a separate thread and run it more frequently, every x milliseconds(out side of updateStats), if there are any requests (with deduplication). // Example var dedupedOps void schedule(Op op) // dedup/add op to dedupedOps. void start(){ while(true) } |
1f41b31
to
70f1883
Compare
Updated the code to check the single subscription. |
70f1883
to
559194d
Compare
…dCursor(single subscription check) from subscribe
559194d
to
3cb8c55
Compare
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
@heesung-sn Please change the PR title.
|
Oops, thanks. Updated the title. |
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.
👍
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 your contribution @heesung-sn! Merging..
…dCursor(single subscription check) upon subscription (#19343)
…dCursor(single subscription check) upon subscription (#19343)
…dCursor(single subscription check) upon subscription (apache#19343) (cherry picked from commit 6b067a5)
Motivation
Related to #19341,
checkBackloggedCursors()
iterates all subscriptions for each subscribe call.checkBackloggedCursors()
in the subscription path is redundant ascheckBackloggedCursors()
is called inupdateStats()
everystatsUpdateFrequencyInSecs
(by default 1min).Modifications
Replaced
checkBackloggedCursors()
withcheckBackloggedCursor()
insubscribe()
call. Upon the subscription creation, this will only check thebackloggedCursor
from the subscription to create instead of checking all the subscriptions.Verifying this change
This change can be verified as follows:
simulate many concurrent subscriptions
before this PR
after this PR
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: heesung-sn#24