Skip to content

Conversation

@poorbarcode
Copy link
Contributor

@poorbarcode poorbarcode commented Apr 24, 2025

Motivation

background

Issue

  • The condition of the log should be previousLedger == @param-newLedger, but it is @param-newLedger == @param-newLedger
  • The impact is that there would be invalid error logs for every compaction

Modifications

Fix the wrong condition of printing error log

Documentation

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

Matching PR in forked repository

PR in forked repository: x

@poorbarcode poorbarcode added this to the 4.1.0 milestone Apr 24, 2025
@poorbarcode poorbarcode self-assigned this Apr 24, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Apr 24, 2025
@poorbarcode
Copy link
Contributor Author

/pulsarbot rerun-failure-checks

@poorbarcode poorbarcode changed the title [fix][log]Fix compaction service log's wrong condition [fix][broker]Fix compaction service log's wrong condition Apr 24, 2025
@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 74.25%. Comparing base (bbc6224) to head (851d05b).
Report is 1038 commits behind head on master.

Files with missing lines Patch % Lines
...g/apache/pulsar/compaction/CompactedTopicImpl.java 50.00% 3 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #24207      +/-   ##
============================================
+ Coverage     73.57%   74.25%   +0.67%     
+ Complexity    32624    32528      -96     
============================================
  Files          1877     1865      -12     
  Lines        139502   144659    +5157     
  Branches      15299    16523    +1224     
============================================
+ Hits         102638   107410    +4772     
+ Misses        28908    28757     -151     
- Partials       7956     8492     +536     
Flag Coverage Δ
inttests 26.72% <50.00%> (+2.14%) ⬆️
systests 23.28% <50.00%> (-1.05%) ⬇️
unittests 73.74% <50.00%> (+0.89%) ⬆️

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

Files with missing lines Coverage Δ
...g/apache/pulsar/compaction/CompactedTopicImpl.java 61.53% <50.00%> (-0.89%) ⬇️

... and 1074 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodece nodece requested a review from lhotari April 24, 2025 06:18
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.

The change itself is ok, but the title and description of the PR are not describing the relevant context of the change:

  • The impact isn't described at all. The impact seems to be that there would be invalid error logs for every compaction. Is that correct?
  • This change is a follow up on #23980 changes that haven't been released yet. It would be useful to mention this in the description that this is a follow up on #23980 changes.

@poorbarcode
Copy link
Contributor Author

@lhotari

The impact isn't described at all. The impact seems to be that there would be invalid error logs for every compaction. Is that correct?
This change is a follow up on #23980 changes that haven't been released yet. It would be useful to mention this in the description that this is a follow up on #23980 changes.

Correct, Updated in the Motivation

@poorbarcode poorbarcode requested a review from lhotari April 24, 2025 07:06
@lhotari lhotari changed the title [fix][broker]Fix compaction service log's wrong condition [fix][broker] Fix compaction service log's wrong condition Apr 24, 2025
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

@lhotari lhotari merged commit bc9306f into apache:master Apr 24, 2025
68 of 72 checks passed
lhotari pushed a commit that referenced this pull request Apr 24, 2025
lhotari pushed a commit that referenced this pull request Apr 24, 2025
lhotari pushed a commit that referenced this pull request Apr 24, 2025
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 27, 2025
)

(cherry picked from commit bc9306f)
(cherry picked from commit 2769cbe)
ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request Apr 30, 2025
)

(cherry picked from commit bc9306f)
(cherry picked from commit 2769cbe)
ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request May 1, 2025
)

(cherry picked from commit bc9306f)
(cherry picked from commit 2769cbe)
ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request May 1, 2025
)

(cherry picked from commit bc9306f)
(cherry picked from commit 2769cbe)
ganesh-ctds pushed a commit to datastax/pulsar that referenced this pull request May 1, 2025
)

(cherry picked from commit bc9306f)
(cherry picked from commit 2769cbe)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request May 2, 2025
)

(cherry picked from commit bc9306f)
(cherry picked from commit 2769cbe)
manas-ctds pushed a commit to datastax/pulsar that referenced this pull request May 2, 2025
)

(cherry picked from commit bc9306f)
(cherry picked from commit 63444ed)
srinath-ctds pushed a commit to datastax/pulsar that referenced this pull request May 6, 2025
)

(cherry picked from commit bc9306f)
(cherry picked from commit 63444ed)
nodece pushed a commit to ascentstream/pulsar that referenced this pull request May 28, 2025
KannarFr pushed a commit to CleverCloud/pulsar that referenced this pull request Sep 22, 2025
walkinggo pushed a commit to walkinggo/pulsar that referenced this pull request Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants