Skip to content

[fix][txn] Fix replication of transactional messages - #18896

Merged
nicoloboschi merged 3 commits into
apache:masterfrom
nicoloboschi:fix-replicate-transactions
Dec 13, 2022
Merged

[fix][txn] Fix replication of transactional messages#18896
nicoloboschi merged 3 commits into
apache:masterfrom
nicoloboschi:fix-replicate-transactions

Conversation

@nicoloboschi

Copy link
Copy Markdown
Contributor

Fixes #18888

Motivation

Transaction markers and non committed messages should not being replicated by the geo-replicator.
Those messages must appear as normal messages in the destination clusters.

Modifications

  • Skip txn marker in the replicator
  • Skip messages of aborted txn in the replicator
  • In the replicator read entries from the maxReadPosition of the topic instead of Latest in order to not read non committed messages
  • Remove txn metadata from the message metadata

Notes: I haven't modified the shadow topic replicator because I'm not sure if that feature is completed or not and in any case it's better to fix it in another pull

Documentation

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

Matching PR in forked repository

nicoloboschi#39

@github-actions github-actions Bot added the doc-not-needed Your PR changes do not impact docs label Dec 12, 2022

@eolivelli eolivelli left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@codecov-commenter

codecov-commenter commented Dec 13, 2022

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 6.25000% with 15 lines in your changes missing coverage. Please review.

Project coverage is 47.68%. Comparing base (1be5a69) to head (9cbce38).
Report is 1982 commits behind head on master.

Files with missing lines Patch % Lines
...er/service/persistent/GeoPersistentReplicator.java 0.00% 15 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##             master   #18896       +/-   ##
=============================================
+ Coverage     33.99%   47.68%   +13.68%     
- Complexity     6476    10665     +4189     
=============================================
  Files           623      703       +80     
  Lines         59103    68877     +9774     
  Branches       6147     7386     +1239     
=============================================
+ Hits          20095    32847    +12752     
+ Misses        36347    32349     -3998     
- Partials       2661     3681     +1020     
Flag Coverage Δ
unittests 47.68% <6.25%> (+13.68%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...roker/service/persistent/PersistentReplicator.java 24.31% <100.00%> (+24.31%) ⬆️
...er/service/persistent/GeoPersistentReplicator.java 4.00% <0.00%> (+4.00%) ⬆️

... and 233 files with indirect coverage changes

@lhotari lhotari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good work @nicoloboschi !

@nicoloboschi
nicoloboschi merged commit 83922c2 into apache:master Dec 13, 2022
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Dec 13, 2022
nicoloboschi added a commit to datastax/pulsar that referenced this pull request Dec 13, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 26, 2022
Demogorgon314 pushed a commit to Demogorgon314/pulsar that referenced this pull request Dec 29, 2022
lifepuzzlefun pushed a commit to lifepuzzlefun/pulsar that referenced this pull request Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/geo-replication area/transaction doc-not-needed Your PR changes do not impact docs type/bug The PR fixed a bug or issue reported a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Transactional messages are not geo-replicated correctly

5 participants