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

[fix][broker] fix replicated subscriptions for transactional messages #22452

Merged
merged 26 commits into from
May 13, 2024
Merged

[fix][broker] fix replicated subscriptions for transactional messages #22452

merged 26 commits into from
May 13, 2024

Conversation

thetumbled
Copy link
Contributor

@thetumbled thetumbled commented Apr 7, 2024 •
edited

Motivation

In non-transactional production, we update the LastDataMessagePublishedTimestamp when the message is persisted successfully. But in transactional production, we do not update LastDataMessagePublishedTimestamp, which will impact the feature ReplicatedSubscription.

Modifications

Update the LastDataMessagePublishedTimestamp when the max read position move forward.

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#45

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 7, 2024
@thetumbled thetumbled mentioned this pull request Apr 7, 2024
Closed
@thetumbled
Copy link
Contributor Author

PTAL, thanks. @BewareMyPower @poorbarcode @codelipenghui @liangyepianzhou

BewareMyPower
BewareMyPower reviewed Apr 8, 2024
View reviewed changes
Copy link
Contributor

@BewareMyPower BewareMyPower left a comment

Choose a reason for hiding this comment

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

Could you add a test for it?

@thetumbled
Copy link
Contributor Author

Could you add a test for it?

Added, PTAL, thanks.

@liangyepianzhou
Copy link
Contributor

@thetumbled You need to add a test related to transaction, right?

@thetumbled
Copy link
Contributor Author

@thetumbled You need to add a test related to transaction, right?

testUpdateLastDataMessagePublishedTimestampForTransactionalPublish()

@thetumbled thetumbled mentioned this pull request Apr 8, 2024
Closed
15 tasks
@thetumbled thetumbled changed the title [fix] [broker] fix not updating LastDataMessagePublishedTimestamp in transactional production. [fix][broker] fix replicated subscriptions for transactional messages Apr 9, 2024
@thetumbled thetumbled force-pushed the Fix_lastDataMessagePublishedTimestamp branch from e27fd14 to 6fae3a3 Compare April 9, 2024 08:00
@dao-jun
Copy link
Member

dao-jun commented May 8, 2024

@poorbarcode any more change requests?

@coderzc coderzc modified the milestones: 3.3.0, 3.4.0 May 8, 2024
@thetumbled
fix conflict.
0637308
@thetumbled
Merge branch 'refs/heads/master' into Fix_lastDataMessagePublishedTim…
9cc0224 
…estamp

# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/transaction/buffer/impl/TopicTransactionBuffer.java
#	pulsar-broker/src/test/java/org/apache/pulsar/broker/transaction/TransactionTest.java
@thetumbled
fix conflict.
5d1c2ba
@thetumbled
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

1 similar comment
@thetumbled
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@congbobo184
Copy link
Contributor 

Error: org.apache.pulsar.broker.service.ReplicatorSubscriptionWithTransactionTest.testReplicatedSubscriptionWhenReplicatorProducerIsClosed Time elapsed: 12.567 s <<< FAILURE!
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a org.apache.pulsar.broker.service.ReplicatorSubscriptionTest expected object to not be null within 10 seconds.

Pulsar CI / CI - Unit - Brokers - Broker Group 1 (pull_request) test is not stable, please check the reason

@thetumbled
Copy link
Contributor Author 

Error: org.apache.pulsar.broker.service.ReplicatorSubscriptionWithTransactionTest.testReplicatedSubscriptionWhenReplicatorProducerIsClosed Time elapsed: 12.567 s <<< FAILURE!
org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a org.apache.pulsar.broker.service.ReplicatorSubscriptionTest expected object to not be null within 10 seconds.

Pulsar CI / CI - Unit - Brokers - Broker Group 1 (pull_request) test is not stable, please check the reason

I have fixed the test code, please help to trigger the CI again, thanks.

@thetumbled
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

2 similar comments
@thetumbled
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@thetumbled
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@dao-jun
Copy link
Member

dao-jun commented May 13, 2024

@poorbarcode any more change requests?

@thetumbled
Copy link
Contributor Author

PTAL, thanks. @poorbarcode

@poorbarcode poorbarcode merged commit 9fd1b61 into apache:master May 13, 2024
50 checks passed
@lhotari
Copy link
Member

lhotari commented May 14, 2024

cherry-picking this to branch-3.0 . it looks like #21816 and #22656 need to be cherry-picked before this one to reduce merge conflicts.

@thetumbled
Copy link
Contributor Author

cherry-picking this to branch-3.0 . it looks like #21816 and #22656 need to be cherry-picked before this one to reduce merge conflicts.

Hi, Lari. You can cherry pick #22656 into branch-3.0 without conflict, and i can help to cherry pick this pr if there are any conflict.

@lhotari
Copy link
Member

lhotari commented May 14, 2024

I'm giving up on cherry-picking #21816 since that's too large change. I'll check what the conflicts are when applying only #22656 and #22452.

lhotari pushed a commit that referenced this pull request May 14, 2024
@thetumbled @lhotari
[fix][broker] fix replicated subscriptions for transactional messages (…
e300fbd 
…#22452)

(cherry picked from commit 9fd1b61)
lhotari added a commit that referenced this pull request May 14, 2024
@lhotari
[fix][broker] fix replicated subscriptions for transactional messages (…
e99bf9a 
…#22452)

(cherry picked from commit 9fd1b61)

# Conflicts:
#	pulsar-broker/src/main/java/org/apache/pulsar/broker/service/persistent/PersistentTopic.java
nikhil-ctds pushed a commit to datastax/pulsar that referenced this pull request May 15, 2024
@thetumbled @nikhil-ctds
[fix][broker] fix replicated subscriptions for transactional messages (…
d7b24e9 
…apache#22452)

(cherry picked from commit 9fd1b61)
(cherry picked from commit e300fbd)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request May 16, 2024
@thetumbled @srinath-ctds
[fix][broker] fix replicated subscriptions for transactional messages (…
99c2a9d 
…apache#22452)

(cherry picked from commit 9fd1b61)
(cherry picked from commit e300fbd)
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

@codelipenghui codelipenghui codelipenghui left review comments

@BewareMyPower BewareMyPower BewareMyPower left review comments

@lhotari lhotari lhotari approved these changes

@dao-jun dao-jun dao-jun approved these changes

@poorbarcode poorbarcode poorbarcode approved these changes

@congbobo184 congbobo184 congbobo184 approved these changes

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@thetumbled thetumbled

Labels
area/geo-replication cherry-picked/branch-3.0 cherry-picked/branch-3.2 doc-not-needed Your PR changes do not impact docs ready-to-test release/3.0.5 release/3.2.3 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Milestone
3.4.0
Development

Successfully merging this pull request may close these issues.

None yet