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.

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

[fix][broker] Add expire check for replicator #23975

Merged
merged 2 commits into from
Feb 28, 2025

Conversation

nodece
Copy link
Member

@nodece nodece commented Feb 13, 2025

Motivation

#21865 removes the expire check for the replicator and compaction subscription. However, this introduces a breaking change in that the replication message never expires.

The TTL policy should be applied to the subscription and replicator.

Modifications

  • Add expire check for the replicator.

Documentation

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

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 13, 2025
@nodece nodece self-assigned this Feb 13, 2025
@nodece nodece added this to the 4.1.0 milestone Feb 13, 2025
@nodece nodece added type/bug The PR fixed a bug or issue reported a bug area/broker labels Feb 13, 2025
@nodece nodece requested a review from rdhabalia February 18, 2025 03:45
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, just wondering whether we need more comments in the testReplicatorWithTTL test case so that it's clear what the expected behavior is. It's currently hard to validate the test case itself.

@lhotari lhotari requested a review from merlimat February 28, 2025 07:41
@nodece nodece force-pushed the fix-replicator-expire-check branch from fcaf3f0 to c58deee Compare February 28, 2025 08:41
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for improving the test case too

@codecov-commenter
Copy link

codecov-commenter commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 40.90909% with 13 lines in your changes missing coverage. Please review.

Project coverage is 74.23%. Comparing base (bbc6224) to head (c58deee).
Report is 943 commits behind head on master.

Files with missing lines Patch % Lines
...ar/broker/service/persistent/ShadowReplicator.java 0.00% 8 Missing and 1 partial ⚠️
...er/service/persistent/GeoPersistentReplicator.java 66.66% 2 Missing and 1 partial ⚠️
...sar/broker/service/persistent/PersistentTopic.java 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #23975      +/-   ##
============================================
+ Coverage     73.57%   74.23%   +0.65%     
+ Complexity    32624    32401     -223     
============================================
  Files          1877     1861      -16     
  Lines        139502   144192    +4690     
  Branches      15299    16424    +1125     
============================================
+ Hits         102638   107037    +4399     
+ Misses        28908    28711     -197     
- Partials       7956     8444     +488     
Flag Coverage Δ
inttests 26.65% <0.00%> (+2.07%) ⬆️
systests 23.12% <0.00%> (-1.20%) ⬇️
unittests 73.73% <40.90%> (+0.89%) ⬆️

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

Files with missing lines Coverage Δ
...sar/broker/service/persistent/PersistentTopic.java 79.67% <75.00%> (+1.21%) ⬆️
...er/service/persistent/GeoPersistentReplicator.java 75.40% <66.66%> (-2.62%) ⬇️
...ar/broker/service/persistent/ShadowReplicator.java 51.85% <0.00%> (-6.69%) ⬇️

... and 1053 files with indirect coverage changes

@nodece nodece merged commit d0025e7 into apache:master Feb 28, 2025
53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker doc-not-needed Your PR changes do not impact docs release/3.0.11 release/3.3.6 release/4.0.4 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.

3 participants