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]stage 1: check the cursor status when handling flowPermits #16546

Closed
wants to merge 3 commits into from

Conversation

HQebupt
Copy link
Contributor

@HQebupt HQebupt commented Jul 12, 2022

Fixes #16757

Motivation

also fixes #14559

Reader belongs to exclusive subscription type, and it uses nonDurable cursor. After receiving messages, Reader will ack cumulatively immediately.
The flowPermits are triggered in multiple scenarios from the client side and it is isolated from seek of Consumer. Therefore, it is possibile that flowPermits will execute after seek from the client side, like the following flow chart.

image

When handleSeek processing is delay from the server side, the MarkDelete position is modified in a wrong way.
The expected result is that Readercan re-consume messages from mark delete:(1,1) after seek. But it doesn't work.

Modifications

  • stage 1: It is necessary to check the current cursor status when handling flowPermits from the server side.
  • stage 2: check the seek status before ack in ConsumerImpl, in which prevent an in-process entry reading operation after the seek position
    @codelipenghui

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 and label this PR (if you have committer privilege).

Need to update docs?

  • doc-not-needed

@HQebupt HQebupt force-pushed the resetCursorInProgress branch from 5cc8392 to 2d5627b Compare July 12, 2022 14:10
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Jul 12, 2022
@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 13, 2022

/pulsarbot run-failure-checks

2 similar comments
@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 13, 2022

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 13, 2022

/pulsarbot run-failure-checks

@codelipenghui codelipenghui added this to the 2.11.0 milestone Jul 13, 2022
@codelipenghui codelipenghui added type/bug The PR fixed a bug or issue reported a bug area/broker release/2.10.2 release/2.9.4 labels Jul 13, 2022
@HQebupt HQebupt changed the title [fix][broker]check the cursor status when handling flowPermits [fix][broker]stage 1: check the cursor status when handling flowPermits Jul 13, 2022
@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 13, 2022

/pulsarbot run-failure-checks

7 similar comments
@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 13, 2022

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 13, 2022

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 14, 2022

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 14, 2022

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 15, 2022

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 22, 2022

/pulsarbot run-failure-checks

@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 24, 2022

/pulsarbot run-failure-checks

@HQebupt HQebupt force-pushed the resetCursorInProgress branch from fb274c8 to fd05b8d Compare July 24, 2022 07:08
@HQebupt
Copy link
Contributor Author

HQebupt commented Jul 24, 2022

/pulsarbot run-failure-checks

@codelipenghui codelipenghui modified the milestones: 2.11.0, 2.12.0 Jul 26, 2022
@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Aug 25, 2022
@Jason918
Copy link
Contributor

@HQebupt Please fix CI failure.

@HQebupt
Copy link
Contributor Author

HQebupt commented Aug 28, 2022

/pulsarbot run-failure-checks

1 similar comment
@HQebupt
Copy link
Contributor Author

HQebupt commented Aug 28, 2022

/pulsarbot run-failure-checks

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@congbobo184
Copy link
Contributor

@HQebupt hi, I move this PR to release/2.9.5, if you have any questions, please ping me. thanks.

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Dec 19, 2022
@HQebupt HQebupt closed this Dec 28, 2022
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/2.9.5 release/2.10.3 Stale type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
4 participants