[ML] Fix race condition in updating lastMarkDeleteEntry field by lhotari · Pull Request #15031 · apache/pulsar

lhotari · 2022-04-05T14:52:11Z

Motivation

missed updates to lastMarkDeleteEntry can lead to the subscription and consuming getting stuck. This seems to impact both Key_shared and Shared subscriptions.

Modifications

when updating lastMarkDeleteEntry field, make sure that the new value is later than the previous value.

- missed updates can lead to the subscription and consuming getting stuck

eolivelli · 2022-04-05T14:54:34Z

+                                                   final Map<String, Long> properties) {
+        LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+            if (last != null && last.newPosition.compareTo(newPosition) > 0) {
+                // keep current value, don't update


what about adding a DEBUG statement here ?

let's address logging in the previous case and use a similar resolution here

eolivelli · 2022-04-05T14:54:43Z

-        lastMarkDeleteEntry = mdEntry;
+        LAST_MARK_DELETE_ENTRY_UPDATER.updateAndGet(this, last -> {
+            if (last != null && last.newPosition.compareTo(mdEntry.newPosition) > 0) {
+                // keep the current value since it's later then the mdEntry.newPosition


what about adding a DEBUG statement here ?

there would be a lot of logging since it's very common that it would happen.

then we can add DEBUG in the other branch, the "new" code that you are adding

Do you mean something like this?

if (log.isDebugEnabled()) { log.debug("Keeping {} since it's newer than {}", last, mdEntry); }

or is this better

if (log.isDebugEnabled()) { log.debug("Ignoring update {} to lastMarkDeleteEntry {} since current value is later", mdEntry, last); }

lhotari · 2022-04-05T14:57:41Z

btw. The problem scenario looked like very similar as in #14261. However there was a difference.

dave2wave

LGTM

merlimat

👍

eolivelli

Lgtm

…#15031) - missed updates can lead to the subscription and consuming getting stuck (cherry picked from commit ad2f397) (cherry picked from commit f657bf8)

…#15031) - missed updates can lead to the subscription and consuming getting stuck (cherry picked from commit ad2f397) (cherry picked from commit 6f74686)

…#15031) - missed updates can lead to the subscription and consuming getting stuck

…apache#15031)" This reverts commit ad2f397.

lhotari · 2022-04-07T16:21:50Z

I made a follow-up PR #15067 to improve the solution. Please review.

…15067) - follow up on #15031 * [ML] Fix race in persisting mark delete position * [ML] Resetting should reset lastMarkDeleteEntry * [ML] Reset fields in initializeCursorPosition method

…15067) - follow up on #15031 * [ML] Fix race in persisting mark delete position * [ML] Resetting should reset lastMarkDeleteEntry * [ML] Reset fields in initializeCursorPosition method (cherry picked from commit a19a30a)

(apache#15067) - follow up on apache#15031 * [ML] Fix race in persisting mark delete position * [ML] Resetting should reset lastMarkDeleteEntry * [ML] Reset fields in initializeCursorPosition method (cherry picked from commit a19a30a)

(apache#15067) - follow up on apache#15031 * [ML] Fix race in persisting mark delete position * [ML] Resetting should reset lastMarkDeleteEntry * [ML] Reset fields in initializeCursorPosition method

…#15031) - missed updates can lead to the subscription and consuming getting stuck

(apache#15067) - follow up on apache#15031 * [ML] Fix race in persisting mark delete position * [ML] Resetting should reset lastMarkDeleteEntry * [ML] Reset fields in initializeCursorPosition method

Technoboy- · 2022-04-22T04:42:24Z

Why does this patch not add a test ?
@eolivelli @lhotari

…#15031) (ad2f397) - missed updates can lead to the subscription and consuming getting stuck

[ML] Fix race condition in updating lastMarkDeleteEntry field

9ff1879

- missed updates can lead to the subscription and consuming getting stuck

lhotari added type/bug The PR fixed a bug or issue reported a bug area/broker labels Apr 5, 2022

lhotari requested review from 315157973, aloyszhang, codelipenghui, dave2wave, dlg99, eolivelli, hangc0276, massakam, merlimat, michaeljmarshall, rdhabalia and yjshen April 5, 2022 14:52

lhotari self-assigned this Apr 5, 2022

github-actions Bot added the doc-label-missing label Apr 5, 2022

lhotari added the doc-not-needed Your PR changes do not impact docs label Apr 5, 2022

apache deleted a comment from github-actions Bot Apr 5, 2022

github-actions Bot removed the doc-label-missing label Apr 5, 2022

apache deleted a comment from github-actions Bot Apr 5, 2022

eolivelli reviewed Apr 5, 2022

View reviewed changes

dave2wave approved these changes Apr 5, 2022

View reviewed changes

lhotari requested review from BewareMyPower and addisonj April 5, 2022 15:23

merlimat approved these changes Apr 5, 2022

View reviewed changes

michaeljmarshall approved these changes Apr 5, 2022

View reviewed changes

eolivelli approved these changes Apr 5, 2022

View reviewed changes

eolivelli merged commit ad2f397 into apache:master Apr 5, 2022

lhotari added cherry-picked/branch-2.7 Archived: 2.7 is end of life cherry-picked/branch-2.8 Archived: 2.8 is end of life cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 labels Apr 6, 2022

Lannnnh pushed a commit to Lannnnh/pulsar that referenced this pull request Apr 6, 2022

[ML] Fix race condition in updating lastMarkDeleteEntry field (apache…

7283eff

…#15031) - missed updates can lead to the subscription and consuming getting stuck

lhotari added a commit to lhotari/pulsar that referenced this pull request Apr 7, 2022

Revert "[ML] Fix race condition in updating lastMarkDeleteEntry field (…

7cd042a

…apache#15031)" This reverts commit ad2f397.

lhotari mentioned this pull request Apr 7, 2022

[ML] Follow up on race condition fixes in ManagedCursorImpl #15031 #15067

Merged

Nicklee007 pushed a commit to Nicklee007/pulsar that referenced this pull request Apr 20, 2022

[ML] Fix race condition in updating lastMarkDeleteEntry field (apache…

99cd3ea

…#15031) - missed updates can lead to the subscription and consuming getting stuck

Technoboy- mentioned this pull request Jul 25, 2022

Failed to flush mark-delete position #16237

Closed

Jason918 added release/2.7.5 and removed release/2.7.6 labels Jul 29, 2022

Shawyeok pushed a commit to Shawyeok/pulsar that referenced this pull request Sep 6, 2022

[ML] Fix race condition in updating lastMarkDeleteEntry field (apache…

c0011ed

…#15031) (ad2f397) - missed updates can lead to the subscription and consuming getting stuck

michaeljmarshall mentioned this pull request Oct 28, 2022

[fix][ml] Persist correct markDeletePosition to prevent message loss #18237

Merged

1 task

wolfstudy mentioned this pull request Dec 28, 2022

[Bug] MarkDeletePosition causes the corresponding ledger to not be deleted in borderline cases #19077

Open

2 tasks

Uh oh!

Conversation

lhotari commented Apr 5, 2022

Motivation

Modifications

Uh oh!

eolivelli Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

eolivelli Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

eolivelli Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

lhotari commented Apr 5, 2022

Uh oh!

dave2wave left a comment

Choose a reason for hiding this comment

Uh oh!

merlimat left a comment

Choose a reason for hiding this comment

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

lhotari commented Apr 7, 2022

Uh oh!

Technoboy- commented Apr 22, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants