[fix] [broker] fix startNewSnapshot when lastDataMessagePublishedTimestamp not change.

[thetumbled]

Motivation

#16618 introduce a feature that we could skip creating a subscription replication snapshot if no messages have been published.
So it is reasonable that if lastDataMessagePublishedTimestamp not change, we should skip taking snapshot.
But it don't check for equality, and i don't get the explanation:
#16618 (comment)

Modifications

Add the check for equality.

Verifying this change

• Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

• Dependencies (add or upgrade a dependency)
• The public API
• The schema
• The default values of configurations
• The threading model
• The binary protocol
• The REST endpoints
• The admin CLI options
• The metrics
• Anything that affects deployment

Documentation

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

Matching PR in forked repository

PR in forked repository: thetumbled#47

[thetumbled]

Could you help to review this pr? @lhotari @codelipenghui @BewareMyPower @dao-jun @merlimat

[lhotari]

@thetumbled I've been trying to get a review for PR #16651 for quite some time without response. It's slightly related. Can you please check it?

[lhotari]

But it don't check for equality, and i don't get the explanation:
#16618 (comment)

I'll explain. In the current code in master branch,

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/ReplicatedSubscriptionsController.java

Lines 209 to 210 in a51bbdd

	if (topic.getLastDataMessagePublishedTimestamp() < lastCompletedSnapshotStartTime
	|| topic.getLastDataMessagePublishedTimestamp() == 0) {

.

The condition topic.getLastDataMessagePublishedTimestamp() < lastCompletedSnapshotStartTime is comparing 2 different types of timestamps: the first one is the timestamp of publishing the last message. The other one is the completion of the snapshot starting time. Checking for equality isn't necessary since those timestamps won't be the same. Does this explanation clarify that?

The problem of duplicate snapshots is solved by #16651.

[lhotari]

I don't think that this change makes a difference. I explained it in comments. I have created another PR a long time ago to eliminate unnecessary snapshots, that is #16651

[thetumbled]

But it don't check for equality, and i don't get the explanation:
#16618 (comment)

I'll explain. In the current code in master branch,

pulsar/pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/ReplicatedSubscriptionsController.java

Lines 209 to 210 in a51bbdd

	if (topic.getLastDataMessagePublishedTimestamp() < lastCompletedSnapshotStartTime
	|| topic.getLastDataMessagePublishedTimestamp() == 0) {

.
The condition topic.getLastDataMessagePublishedTimestamp() < lastCompletedSnapshotStartTime is comparing 2 different types of timestamps: the first one is the timestamp of publishing the last message. The other one is the completion of the snapshot starting time. Checking for equality isn't necessary since those timestamps won't be the same. Does this explanation clarify that?

The problem of duplicate snapshots is solved by #16651.

I get it. thanks for explanation!

[thetumbled]

@thetumbled I've been trying to get a review for PR #16651 for quite some time without response. It's slightly related. Can you please check it?

I am not that familiar with ReplicatedSubscription implementation, but try to solve another problem related to it. #22452
Maybe you could refer for @merlimat