Skip to content

[fix] [broker] Prevent long deduplication cursor backlog so that topic loading wouldn't timeout#22479

Merged
poorbarcode merged 8 commits into
apache:masterfrom
poorbarcode:fix/cursor_read_pos_miss
Apr 14, 2024
Merged

[fix] [broker] Prevent long deduplication cursor backlog so that topic loading wouldn't timeout#22479
poorbarcode merged 8 commits into
apache:masterfrom
poorbarcode:fix/cursor_read_pos_miss

Conversation

@poorbarcode

@poorbarcode poorbarcode commented Apr 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

Background 1

  • When a topic is loading up, it will wait for the deduplication recovery to be finished if deduplication is enabled.
    • users can enable deduplication on the broker level, namespace level or topic level.
  • There is a scheduled task to acknowledge messages for the deduplication cursor.

Issue 1
(Highlight) the scheduled task that acknowledges messages for the deduplication cursor can only be created when the broker level deduplication is enabled, leading to an infinite backlog of deduplication cursor; the next deduplication recovery after a topic unloading will take a long time, which leads to topic load timeout.

Background 2

  • MessageDeduplication maintains a counter that indicates how many messages have not been taken snapshot yet, it increases after per add entry. Broker triggers a snapshot building once the counter is greater than the config brokerDeduplicationEntriesInterval(default value is 1000).

Issue 2, thanks for @mattisonchao 's reminding ❤️

  • send 999 messages, the counter increases to 999, does not reach brokerDeduplicationEntriesInterval(1000) now.
  • unload the topic, the counter will be reset to 0,
  • send 999 messages, the counter increases to 999, does not reach brokerDeduplicationEntriesInterval(1000) now.
  • ...
  • So the snapshot will never be built, resulting the same issue as the scenario 1.

A topic internal stats
topic-internal-stats.txt

Modifications

  • Always start the scheduled task.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost release/2.10.7 release/2.11.5 release/3.2.3 release/3.0.5 labels Apr 11, 2024
@poorbarcode poorbarcode added this to the 3.3.0 milestone Apr 11, 2024
@poorbarcode poorbarcode self-assigned this Apr 11, 2024
@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Apr 11, 2024
mattisonchao
mattisonchao reviewed Apr 11, 2024
View reviewed changes
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated
mattisonchao
mattisonchao reviewed Apr 11, 2024
View reviewed changes
Comment thread pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java Outdated
lhotari
lhotari reviewed Apr 11, 2024
View reviewed changes
Comment thread pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java Outdated
@lhotari

lhotari commented Apr 11, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

The title "Deduplication cursor has infinite backlog causing topic load timeout" is misleading since the backlog isn't infinite.
This fix doesn't really address the case when there's a long backlog at the time of topic loading. It's trying to prevent the long backlog from building up. Perhaps "Prevent long deduplication cursor backlog so that topic loading wouldn't timeout" would be a better title?

@poorbarcode poorbarcode changed the title [fix] [broker] Deduplication cursor has infinite backlog causing topic load timeout [fix] [broker] Prevent long deduplication cursor backlog so that topic loading wouldn't timeout Apr 11, 2024
@poorbarcode

poorbarcode commented Apr 11, 2024

Copy link
Copy Markdown
Contributor Author

The title "Deduplication cursor has infinite backlog causing topic load timeout" is misleading since the backlog isn't infinite.
This fix doesn't really address the case when there's a long backlog at the time of topic loading. It's trying to prevent the long backlog from building up. Perhaps "Prevent long deduplication cursor backlog so that topic loading wouldn't timeout" would be a better title?

Done

@poorbarcode poorbarcode mentioned this pull request Apr 11, 2024
Closed
4 tasks
@mattisonchao mattisonchao reopened this Apr 11, 2024
@kristgpt

kristgpt commented Apr 13, 2024

Copy link
Copy Markdown

👍

@poorbarcode poorbarcode merged commit 837f8bc into apache:master Apr 14, 2024
poorbarcode added a commit that referenced this pull request Apr 15, 2024
@poorbarcode
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
7cec82f 
…c loading wouldn't timeout (#22479)

(cherry picked from commit 837f8bc)
poorbarcode added a commit that referenced this pull request Apr 15, 2024
@poorbarcode
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
0fbcbb2 
…c loading wouldn't timeout (#22479)

(cherry picked from commit 837f8bc)
poorbarcode added a commit that referenced this pull request Apr 15, 2024
@poorbarcode
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
97518ee 
…c loading wouldn't timeout (#22479)

(cherry picked from commit 837f8bc)
poorbarcode added a commit that referenced this pull request Apr 15, 2024
@poorbarcode
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
6855405 
…c loading wouldn't timeout (#22479)

(cherry picked from commit 837f8bc)
poorbarcode added a commit that referenced this pull request Apr 15, 2024
@poorbarcode
Revert "[fix] [broker] Prevent long deduplication cursor backlog so t…
f91b519 
…hat topic loading wouldn't timeout (#22479)"

This reverts commit 7cec82f.
poorbarcode added a commit that referenced this pull request Apr 15, 2024
@poorbarcode
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
f5c853a 
…c loading wouldn't timeout (#22479)

(cherry picked from commit 837f8bc)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 16, 2024
@poorbarcode @mukesh-ctds
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
fa262d3 
…c loading wouldn't timeout (apache#22479)

(cherry picked from commit 837f8bc)
(cherry picked from commit 0fbcbb2)
@poorbarcode

poorbarcode commented Apr 17, 2024

Copy link
Copy Markdown
Contributor Author

This may leave BrokerDeduplicationEnabled not working, and we also have admin-API for getting/setting this value

Do not worry, it only affects the scheduled task.

mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
@poorbarcode @mukesh-ctds
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
60eee79 
…c loading wouldn't timeout (apache#22479)

(cherry picked from commit 837f8bc)
(cherry picked from commit 0fbcbb2)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 17, 2024
@poorbarcode @mukesh-ctds
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
8b345b1 
…c loading wouldn't timeout (apache#22479)

(cherry picked from commit 837f8bc)
(cherry picked from commit 0fbcbb2)
mukesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 19, 2024
@poorbarcode @mukesh-ctds
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
5158efb 
…c loading wouldn't timeout (apache#22479)

(cherry picked from commit 837f8bc)
(cherry picked from commit 0fbcbb2)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 23, 2024
@poorbarcode @srinath-ctds
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
ca62ee3 
…c loading wouldn't timeout (apache#22479)

(cherry picked from commit 837f8bc)
(cherry picked from commit 0fbcbb2)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request May 13, 2024
@poorbarcode @nodece
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
4c98ba5 
…c loading wouldn't timeout (apache#22479)

(cherry picked from commit 837f8bc)
@lhotari

lhotari commented May 14, 2024

Copy link
Copy Markdown
Member

@poorbarcode DeduplicationDisabledBrokerLevelTest.testSnapshotCounterAfterUnload fails in branch-3.2 and blocks 3.2.3 release. do you have a chance to fix the problem in branch-3.2 ?

@poorbarcode

poorbarcode commented May 14, 2024

Copy link
Copy Markdown
Contributor Author

@poorbarcode DeduplicationDisabledBrokerLevelTest.testSnapshotCounterAfterUnload fails in branch-3.2 and blocks 3.2.3 release. do you have a chance to fix the problem in branch-3.2 ?

@lhotari found the issue. PR 22034 exists in branch-3.2 so the test is wrong. resolved with commit c2532b9

@TakaHiR07 TakaHiR07 mentioned this pull request Jul 4, 2024
Closed
3 tasks
hanmz pushed a commit to hanmz/pulsar that referenced this pull request Feb 12, 2025
@poorbarcode @hanmz
[fix] [broker] Prevent long deduplication cursor backlog so that topi…
1222901 
…c loading wouldn't timeout (apache#22479)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shibd shibd shibd approved these changes

@mattisonchao mattisonchao mattisonchao approved these changes

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@lhotari lhotari Awaiting requested review from lhotari

+1 more reviewer

@kristgpt kristgpt kristgpt approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@poorbarcode poorbarcode

Labels

category/reliability The function does not work properly in certain specific environments or failures. e.g. data lost cherry-picked/branch-2.10 cherry-picked/branch-2.11 cherry-picked/branch-3.0 cherry-picked/branch-3.2 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.10.7 release/2.11.5 release/3.0.5 release/3.2.3

Projects

None yet

Milestone

3.3.0

Development

Successfully merging this pull request may close these issues.

6 participants

@poorbarcode @lhotari @kristgpt @shibd @mattisonchao @coderzc