Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Skip creating a subscription replication snapshot if no messages have been published after the topic gets activated on a broker #16618

Merged
merged 2 commits into from
Sep 27, 2022
Merged

Skip creating a subscription replication snapshot if no messages have been published after the topic gets activated on a broker #16618

merged 2 commits into from
Sep 27, 2022

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Jul 15, 2022 •
edited by github-actions bot
Loading

Motivation

There's a bug in replicated subscriptions snapshotting. Snapshots get continuously created after a topic gets activated and before new messages are published. This is a gap in the logic introduced in #11922.

Additional context

The intention of the #10292 and #11922 changes were to pause replication snapshots when there are no new messages.
These changes were made to address #6437 and as an alternative to #7299 changes.

Modifications

  • Add condition to skip creating snapshots when the lastDataMessagePublishedTimestamp is 0.
  • Modify existing test case testReplicationSnapshotStopWhenNoTraffic to verify that snapshots aren't published before messages have been published.
  • doc-not-needed

@lhotari lhotari added area/broker area/geo-replication doc-not-needed Your PR changes do not impact docs labels Jul 15, 2022
@lhotari lhotari self-assigned this Jul 15, 2022
@lhotari
Copy link
Member Author

lhotari commented Jul 15, 2022

I was also considering a fix where a single snapshot would be created after topic activation instead of skipping the snapshotting completely. @merlimat what is your recommendation for the fix?

@lhotari lhotari force-pushed the lh-skip-creating-repl-snapshot-when-no-messages-published branch from 9091794 to f4e68b2 Compare August 11, 2022 07:17
@lhotari
Copy link
Member Author

lhotari commented Aug 11, 2022

@merlimat @codelipenghui Please review

@lhotari lhotari requested a review from Technoboy- August 11, 2022 07:19
@Jason918
Copy link
Contributor

@merlimat @codelipenghui @Technoboy- PTAL

michaeljmarshall
michaeljmarshall approved these changes Aug 30, 2022
View reviewed changes
Copy link
Member

@michaeljmarshall michaeljmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

While reviewing the code surrounding this PR, I noticed that the lastDataMessagePublishedTimestamp variable is not volatile. It seems like this could lead to behavior where we don't send snapshots as early as expected. Do you think we should make it volatile?

@Technoboy- Technoboy- added this to the 2.12.0 milestone Aug 30, 2022
@lhotari lhotari force-pushed the lh-skip-creating-repl-snapshot-when-no-messages-published branch from f4e68b2 to b58145d Compare September 27, 2022 04:45
@lhotari lhotari merged commit 43ad6f9 into apache:master Sep 27, 2022
@lhotari
Copy link
Member Author

lhotari commented Sep 28, 2022 •
edited
Loading

While reviewing the code surrounding this PR, I noticed that the lastDataMessagePublishedTimestamp variable is not volatile. It seems like this could lead to behavior where we don't send snapshots as early as expected. Do you think we should make it volatile?

@michaeljmarshall Good point. I'll cover that as part of #17252.

@congbobo184 congbobo184 added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 10, 2022
congbobo184 pushed a commit that referenced this pull request Nov 10, 2022
@lhotari
Skip creating a subscription replication snapshot if no messages have…
4d4f189 
… been published after the topic gets activated on a broker (#16618)

* Skip creating a replication snapshot if no messages have been published

* Adapt test to new behavior where replication snapshots happen only when there are new messages

(cherry picked from commit 43ad6f9)
congbobo184 pushed a commit that referenced this pull request Dec 1, 2022
@lhotari
Skip creating a subscription replication snapshot if no messages have…
594ee5b 
… been published after the topic gets activated on a broker (#16618)

* Skip creating a replication snapshot if no messages have been published

* Adapt test to new behavior where replication snapshots happen only when there are new messages

(cherry picked from commit 43ad6f9)
liangyepianzhou pushed a commit that referenced this pull request Dec 12, 2022
@lhotari @liangyepianzhou
Skip creating a subscription replication snapshot if no messages have…
eac65c0 
… been published after the topic gets activated on a broker (#16618)

* Skip creating a replication snapshot if no messages have been published

* Adapt test to new behavior where replication snapshots happen only when there are new messages

(cherry picked from commit 43ad6f9)
liangyepianzhou added a commit that referenced this pull request Dec 12, 2022
@liangyepianzhou
[cherry-pick][branch-2.10] fix test after cherry-pick #16618
f056d9d
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
@lhotari @nicoloboschi
Skip creating a subscription replication snapshot if no messages have…
b924883 
… been published after the topic gets activated on a broker (apache#16618)

* Skip creating a replication snapshot if no messages have been published

* Adapt test to new behavior where replication snapshots happen only when there are new messages

(cherry picked from commit 43ad6f9)
(cherry picked from commit eac65c0)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 10, 2023
@liangyepianzhou @nicoloboschi
[cherry-pick][branch-2.10] fix test after cherry-pick apache#16618
e375630 
(cherry picked from commit f056d9d)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
@lhotari @nicoloboschi
Skip creating a subscription replication snapshot if no messages have…
376b68b 
… been published after the topic gets activated on a broker (apache#16618)

* Skip creating a replication snapshot if no messages have been published

* Adapt test to new behavior where replication snapshots happen only when there are new messages

(cherry picked from commit 43ad6f9)
(cherry picked from commit eac65c0)
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Jan 11, 2023
@liangyepianzhou @nicoloboschi
[cherry-pick][branch-2.10] fix test after cherry-pick apache#16618
761bbc6 
(cherry picked from commit f056d9d)
@thetumbled thetumbled mentioned this pull request Apr 8, 2024
Closed
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Technoboy- Technoboy- Technoboy- left review comments

@michaeljmarshall michaeljmarshall michaeljmarshall approved these changes

@eolivelli eolivelli eolivelli approved these changes

@merlimat merlimat Awaiting requested review from merlimat

@codelipenghui codelipenghui Awaiting requested review from codelipenghui

@massakam massakam Awaiting requested review from massakam

Assignees

@lhotari lhotari

Labels
area/broker area/geo-replication cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 doc-not-needed Your PR changes do not impact docs release/2.8.5 release/2.9.4 release/2.10.3
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
8 participants
@lhotari @Jason918 @Technoboy- @eolivelli @michaeljmarshall @BewareMyPower @congbobo184 @liangyepianzhou