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] Fix can't stop phase-two of compaction even though messageId read reaches lastReadId #20988

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

coderzc
Copy link
Member

@coderzc coderzc commented Aug 14, 2023

Motivation

Currently, the compactor can't stop phase-two even though messageId read reaches lastReadId, this may lead to the next entry being dropped because it does not exist in the latestForKey.

Modifications

Stop phase two when the messageId read reaches lastReadId.

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.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

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:

@@ -277,6 +277,7 @@ private void phaseTwoLoop(RawReader reader, MessageId to, Map<String, MessageId>
promise.complete(null);
}
});
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

What if some of the previous addToCompactedLedger aync write fail later, but the last id's addToCompactedLedger is complete first? Do we mark the promise as successful here?

Don't we need to wait for all outstanding addToCompactedLedger to be complete before making promise complete?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are right, but this PR is to fix phase two can't be stopped. We can open another PR to fix this issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

@heesung-sn I opened #21067 to fix it.

@coderzc coderzc closed this Aug 15, 2023
@coderzc coderzc reopened this Aug 15, 2023
@Technoboy- Technoboy- closed this Aug 20, 2023
@Technoboy- Technoboy- reopened this Aug 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2023

Codecov Report

Merging #20988 (3c16186) into master (9862884) will increase coverage by 3.78%.
Report is 20 commits behind head on master.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #20988      +/-   ##
============================================
+ Coverage     33.53%   37.31%   +3.78%     
- Complexity    12174    12468     +294     
============================================
  Files          1621     1698      +77     
  Lines        126919   131291    +4372     
  Branches      13851    14524     +673     
============================================
+ Hits          42561    48991    +6430     
+ Misses        78745    75856    -2889     
- Partials       5613     6444     +831     
Flag Coverage Δ
inttests 24.60% <0.00%> (+0.32%) ⬆️
systests 25.16% <0.00%> (?)
unittests 32.52% <0.00%> (+0.47%) ⬆️

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

Files Changed Coverage Δ
...rg/apache/pulsar/compaction/TwoPhaseCompactor.java 68.30% <0.00%> (-1.88%) ⬇️

... and 422 files with indirect coverage changes

@coderzc coderzc force-pushed the fix_compaction_stop branch 2 times, most recently from 3de7045 to c2c09a5 Compare August 21, 2023 09:38
@coderzc coderzc force-pushed the fix_compaction_stop branch from 0be467d to 288c73f Compare August 22, 2023 01:12
@coderzc coderzc force-pushed the fix_compaction_stop branch from 288c73f to d789b98 Compare August 22, 2023 06:38
@coderzc coderzc merged commit 9e2195c into apache:master Aug 22, 2023
coderzc added a commit that referenced this pull request Aug 29, 2023
…ageId read reaches lastReadId (#20988)

(cherry picked from commit 9e2195c)
coderzc added a commit that referenced this pull request Aug 29, 2023
…ageId read reaches lastReadId (#20988)

(cherry picked from commit 9e2195c)
coderzc added a commit that referenced this pull request Aug 29, 2023
…ageId read reaches lastReadId (#20988)

(cherry picked from commit 9e2195c)
coderzc added a commit that referenced this pull request Aug 29, 2023
…ageId read reaches lastReadId (#20988)

(cherry picked from commit 9e2195c)
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.

8 participants