pulsar icon indicating copy to clipboard operation
pulsar copied to clipboard

[fix][cursor] Fix enable maxPosition cursor will read in deap loop

Open congbobo184 opened this issue 3 years ago • 6 comments

Motivation

Now we don't check OpAddEntry can read entry is smaller than maxReadPosition, if readPosition is bigger than maxReadPosition, we only can read an empty list as result. In fact, when maxReadPosition have not been updated, we should put read op in to wait state and when maxReadPosition updated, we can notify it and read continue.

Modifications

  1. Add a field named maxReadPosition inManagedLedgerImpl and if read op enable maxReadPosition, we should check this readPosition is bigger than maxReadPosition, if bigger we should put the op into wait state, if smaller or equals we can do this op.
  2. Add a field named waitingCursorsByMaxReadPosition in ManagedLedgerImpl, when cursor has read op in wait state, we can put this read op in to this queue. If maxReadPosition updated, we will pool it and notify this this read op.
  3. In topicTransaction buffer, when any updated maxReadposition op we should sync it to ManagedLedgerImpl maxReadPosition.

Verifying this change

add the test

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

  • Does this pull request introduce a new feature? (yes)
  • If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

congbobo184 avatar Feb 15 '22 03:02 congbobo184

This change is a big refactor in ManagedCursor, it is not a transaction only related patch.

Please change the title, otherwise it is possible that this change would not be noticed by people not interested in Transactions.

Also I would say that we cannot cherry-pick this change to old branches, as it is adding breaking changes in the internal interfaces (I am pretty sure that some Protocol Handler won't compile)

eolivelli avatar Mar 16 '22 16:03 eolivelli

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

github-actions[bot] avatar May 20 '22 02:05 github-actions[bot]

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

github-actions[bot] avatar Jun 19 '22 02:06 github-actions[bot]

I am wondering why this pr do not invoke updateMaxReadPosition in TopicTransactionBuffer#handleTransactionMessage() ?

TakaHiR07 avatar Jan 31 '23 02:01 TakaHiR07

Since we will start the RC version of 3.0.0 on 2023-04-11, I will change the label/milestone of PR who have not been merged.

  • The PR of type feature is deferred to 3.1.0
  • The PR of type fix is deferred to 3.0.1

So drag this PR to 3.0.1

poorbarcode avatar Apr 10 '23 08:04 poorbarcode

@congbobo184 Please add the following content to your PR description and select a checkbox:

- [ ] `doc` <!-- Your PR contains doc changes -->
- [ ] `doc-required` <!-- Your PR changes impact docs and you will update later -->
- [ ] `doc-not-needed` <!-- Your PR changes do not impact docs -->
- [ ] `doc-complete` <!-- Docs have been already added -->

github-actions[bot] avatar Apr 10 '23 08:04 github-actions[bot]

It looks the issue fixed by https://github.com/apache/pulsar/pull/14667, closing... feel free to reopen if this PR still ongoing

dao-jun avatar May 19 '24 15:05 dao-jun

It looks the issue fixed by #14667, closing... feel free to reopen if this PR still ongoing

@dao-jun This issue is not fix. I test again in version-3.0.5 and this issue reproduce. This pr still need to be merged.

TakaHiR07 avatar Jun 13 '24 08:06 TakaHiR07