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

[improve][broker]consumer backlog eviction policy should not reset read position for consumer #18037

Merged
merged 5 commits into from
Oct 19, 2022

Conversation

HQebupt
Copy link
Contributor

@HQebupt HQebupt commented Oct 13, 2022

Motivation

Fixes #18036

Modifications

  • The backlog eviction policy should use asyncMarkDelete instead of resetCursor in order to move the mark delete position.

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)
  • The rest endpoints: (no)
  • The admin cli options: (no)
  • Anything that affects deployment: (no)

Documentation

Check the box below or label this PR directly.

Need to update docs?

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

Matching PR in forked repository

PR in forked repository: HQebupt#5

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Oct 13, 2022
@Technoboy- Technoboy- added this to the 2.12.0 milestone Oct 14, 2022
@Technoboy- Technoboy- added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages area/broker ready-to-test labels Oct 14, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 14, 2022

Codecov Report

❗ No coverage uploaded for pull request base (master@ec74618). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #18037   +/-   ##
=========================================
  Coverage          ?   43.90%           
  Complexity        ?    10604           
=========================================
  Files             ?     1180           
  Lines             ?    84791           
  Branches          ?     9638           
=========================================
  Hits              ?    37229           
  Misses            ?    43882           
  Partials          ?     3680           
Flag Coverage Δ
unittests 43.90% <0.00%> (?)

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

@AnonHxy
Copy link
Contributor

AnonHxy commented Oct 14, 2022

LGTM with minor comment

@AnonHxy
Copy link
Contributor

AnonHxy commented Oct 14, 2022

BacklogQuotaManagerTest.testConsumerBacklogEvictionTimeQuota this test run fail, please check it

@HQebupt
Copy link
Contributor Author

HQebupt commented Oct 14, 2022

BacklogQuotaManagerTest.testConsumerBacklogEvictionTimeQuota this test run fail, please check it

It is due to the test topic and subscription are reused by other test cases, which I have fixed already.

@HQebupt
Copy link
Contributor Author

HQebupt commented Oct 15, 2022

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Oct 16, 2022

Pulsar Flaky tests failed due to #17921

@HQebupt
Copy link
Contributor Author

HQebupt commented Oct 17, 2022

@HQebupt HQebupt changed the title [fix][broker]consumer backlog eviction policy should not reset read position for consumer [improve][broker]consumer backlog eviction policy should not reset read position for consumer Oct 19, 2022
@HQebupt HQebupt merged commit 7404e0d into apache:master Oct 19, 2022
@Technoboy- Technoboy- modified the milestones: 2.12.0, 2.11.0 Nov 4, 2022
Technoboy- pushed a commit that referenced this pull request Nov 4, 2022
…ad position for consumer (#18037)

### Motivation
Fixes #18036

### Modifications
- The backlog eviction policy should use `asyncMarkDelete` instead of `resetCursor` in order to move the mark delete position.
Technoboy- pushed a commit that referenced this pull request Nov 5, 2022
…ad position for consumer (#18037)

Fixes #18036

- The backlog eviction policy should use `asyncMarkDelete` instead of `resetCursor` in order to move the mark delete position.
@Technoboy- Technoboy- added the cherry-picked/branch-2.9 Archived: 2.9 is end of life label Nov 5, 2022
nicoloboschi pushed a commit to datastax/pulsar that referenced this pull request Nov 9, 2022
…ad position for consumer (apache#18037)

Fixes apache#18036

- The backlog eviction policy should use `asyncMarkDelete` instead of `resetCursor` in order to move the mark delete position.

(cherry picked from commit 0b7140b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/broker cherry-picked/branch-2.9 Archived: 2.9 is end of life cherry-picked/branch-2.10 cherry-picked/branch-2.11 doc-not-needed Your PR changes do not impact docs ready-to-test release/2.9.4 release/2.10.3 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.

[Bug] consumer backlog eviction policy should not reset read position of the consumer
5 participants