-
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
[fix] [broker] maintain last active info in memory only. #22794
Conversation
PTAL, thanks. @dao-jun @lhotari @Technoboy- @poorbarcode @coderzc @codelipenghui @BewareMyPower |
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedCursorImpl.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/broker/service/BrokerServiceTest.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.
Good work!
BTW, in your test, after the consumer is closed, and then loading the topic, you only need to verify the sub1.getCursor().getLastActive()
greater than the start time of topic loading.
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, just a minor comment
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
Show resolved
Hide resolved
/pulsarbot rerun-failure-checks |
(cherry picked from commit 2c3909c)
(cherry picked from commit 2c3909c) Signed-off-by: Zixuan Liu <nodeces@gmail.com>
Motivation
PR #17573 has fixed cases that consumer commit offset periodically. It update
lastActive
field of cursor when consumer commit the offset.But there is still corner case that the consumer do not commit the offset for any reason. For example, there is no content in the topic could be read, so the consumer do not commit the offset reasonably. In such case we has no reason delete the subscription.
Anyway, we should be cautious with the delete operation, which could result into data duplication or loss.
This is the second solution trying to solve problem described in #21692.
PR #21692 try to update the last active info every time broker do expiration check.
But strictly speaking, we can't fix all the cases. If we fail to persist the cursor info, the incorrect sub deletion occurs too.
Incorrect deletion of subscription causes serious consequence, such as duplication or data lost.
In my opinion, missing deletion are better than incorrect deletion, so this pr try to avoid all the incorrect deletion even there is posibility of missing deletion.
Modifications
Verifying this change
(Please pick either of the following options)
This change added tests and can be verified as follows:
(example:)
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: thetumbled#54