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

Remove cursor while remove non-durable subscription #5719

Merged

Conversation

@codelipenghui
Copy link
Contributor

codelipenghui commented Nov 21, 2019

Motivation

Remove cursor from cursors of managed ledger while remove non-durable subscription. The data deletion is depends the mark delete position of all cursors, if left a unused cursor in the cursors of managed ledger, data can't be delete as expected.

Modifications

Remove cursor while remove non-durable subscription

Verifying this change

Add new unit test to cover this change

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? (no)
@codelipenghui

This comment has been minimized.

Copy link
Contributor Author

codelipenghui commented Nov 21, 2019

run java8 tests

@sijie
sijie approved these changes Nov 21, 2019
@sijie sijie added the type/bug label Nov 21, 2019
@sijie sijie added this to the 2.4.3 milestone Nov 21, 2019
@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Nov 21, 2019

run java8 tests
run cpp tests

1 similar comment
@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Nov 21, 2019

run java8 tests
run cpp tests

@jiazhai

This comment has been minimized.

Copy link
Contributor

jiazhai commented Nov 22, 2019

run java8 tests
run cpp tests

@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Nov 24, 2019

run cpp tests
run java8 tests

@jiazhai

This comment has been minimized.

Copy link
Contributor

jiazhai commented Nov 25, 2019

The failed tests seems related with this change. and @codelipenghui is working on it.

@codelipenghui

This comment has been minimized.

Copy link
Contributor Author

codelipenghui commented Nov 25, 2019

We need an approach related to seek position, current approach will close all consumers while seek position happens, but this PR will remove the non-durable cursor while all consumers of the non-durable subscription are disconnected. So the reconnected consumer will create a new non-durable cursor using last message id, the unit test can't passed.

It's better to have an approach to handle the position seek, there are two point we need to careful handling:

  1. Consumers acknowledge by outstanding message ids
  2. Avoid consumer to process the outstanding messages

Here, the outstanding messages means messages already dispatched to consumers but the cursor is reset.

For the first point, i think we can use the read position, the read position always larger than the mark delete position except the cursor reset happens, so if the broker ignore the acknowledge requests which the acknowledge position is larger than read position, because there is cursor reset happens.

For the second point, if the consumer cache to many outstanding messages, it's waste of resources if process all of these messages, so it's better to send a cursor reseted notify to consumers before dispatch messages after the cursor reset to consumers, consumers can reset their stat, clear the outstanding messages. currently close consumers also can reset consumer state, clear outstanding messages.

And in #5278, we also have discussed about the reset consumer state when cursor reset happens.

For this PR, i think we can don't close the readers first, and then create a new PR to improve cursor reset since it might need to change the wire protocol.

@codelipenghui codelipenghui modified the milestones: 2.4.3, 2.5.0 Nov 25, 2019
@sijie

This comment has been minimized.

Copy link
Contributor

sijie commented Nov 25, 2019

@codelipenghui shall we move this to 2.5.1 or 2.6.0 release?

@codelipenghui

This comment has been minimized.

Copy link
Contributor Author

codelipenghui commented Nov 26, 2019

Seems @merlimat already have a PR #5022 related to this issue, so we can close this PR

@jiazhai

This comment has been minimized.

Copy link
Contributor

jiazhai commented Nov 28, 2019

run java8 tests

2 similar comments
@jiazhai

This comment has been minimized.

Copy link
Contributor

jiazhai commented Nov 28, 2019

run java8 tests

@jiazhai

This comment has been minimized.

Copy link
Contributor

jiazhai commented Nov 28, 2019

run java8 tests

@sijie
sijie approved these changes Nov 28, 2019
@sijie sijie merged commit 2e30c08 into apache:master Nov 28, 2019
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Jenkins: C++ / Python Tests SUCCESS
Details
Jenkins: Integration Tests SUCCESS
Details
Jenkins: Java 8 - Unit Tests SUCCESS
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.