Skip to content

Conversation

@coderzc
Copy link
Member

@coderzc coderzc commented Jun 30, 2023

Motivation

Currently, some synchronized is used in CompactedTopicImpl to avoid data contention, but it does not completely avoid data contention due to asynchronous operations and may lead to cursor skipping some data.

The cursor may skip some data due to compactionHorizon be updated:

Modifications

  • Remove unnecessary synchronized.
  • Make compactionHorizon unchanged when reading compacted data.
  • Add volatile for compactedTopicContext.

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

Documentation

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

Matching PR in forked repository

PR in forked repository:

@coderzc coderzc changed the title Improve CompactedTopicImpl lock [improve][broker] Improve CompactedTopicImpl lock Jun 30, 2023
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jun 30, 2023
return CompletableFuture.completedFuture(null);
}
if (startPoint == NEWER_THAN_COMPACTED && compactionHorizon.compareTo(cursorPosition) < 0) {
cursor.asyncReadEntriesOrWait(numberOfEntriesToRead, callback, readEntriesCtx,
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need keep this logic ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Condition is always false because it has been judged line-107.

@codelipenghui codelipenghui added this to the 3.1.0 milestone Jul 3, 2023
@codelipenghui codelipenghui added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker labels Jul 3, 2023
@codelipenghui
Copy link
Contributor

/pulsarbot run-failure-checks

@codelipenghui codelipenghui merged commit e231ee9 into apache:master Jul 4, 2023
@dao-jun dao-jun mentioned this pull request Feb 27, 2025
3 tasks
lhotari pushed a commit to lhotari/pulsar that referenced this pull request Feb 28, 2025
lhotari pushed a commit to lhotari/pulsar that referenced this pull request Mar 17, 2025
lhotari pushed a commit to lhotari/pulsar that referenced this pull request Mar 20, 2025
lhotari pushed a commit to lhotari/pulsar that referenced this pull request Mar 26, 2025
lhotari pushed a commit to lhotari/pulsar that referenced this pull request Mar 26, 2025
nodece pushed a commit to nodece/pulsar that referenced this pull request Mar 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/broker area/compaction cherry-picked/branch-3.0 doc-not-needed Your PR changes do not impact docs release/3.0.11 type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants