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

[Broker]Reset cursor with a non-exists position #6120

Merged
merged 3 commits into from
Feb 10, 2020

Conversation

yjshen
Copy link
Member

@yjshen yjshen commented Jan 22, 2020

ManagedCursorImpl.asyncResetCursor is used in three kinds of circumstances:

  • REST API: create a subscription with messageId. Per the document: Reset subscription to message position closest to given position.
  • REST API: reset subscription to a given position: Per the document: Reset subscription to message position closest to given position.
  • Consumer seek command.

In all the cases above, when the user provides a MessageId, we should make the best effort to find the closest position, instead of throwing an InvalidCursorPosition Exception.

This is because if a user provids an invalid position, it's not possible for he or she gets a valid position, since ledger ids for a given topic may not be continuous and only brokers are aware of the order. Therefore, we should avoid throw invalid cursor position but find the nearest position and do the reset stuff.

@yjshen
Copy link
Member Author

yjshen commented Jan 22, 2020

@sijie @jiazhai What do you think of the approach? If we get an agreement on the method, I'll add tests then.

@yjshen yjshen requested review from sijie and jiazhai January 22, 2020 07:02
@jiazhai
Copy link
Member

jiazhai commented Jan 22, 2020

@yjshen this looks good. getNextValidPosition did most of the handling.

Copy link
Member

@jiazhai jiazhai left a comment

Choose a reason for hiding this comment

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

looks good. please go ahead to add tests. and maybe add docs also?

@sijie
Copy link
Member

sijie commented Jan 22, 2020

+1

@sijie sijie added area/admin doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. labels Jan 22, 2020
@yjshen
Copy link
Member Author

yjshen commented Jan 23, 2020

Test added. PTAL, thanks!

@yjshen
Copy link
Member Author

yjshen commented Jan 23, 2020

looks good. please go ahead to add tests. and maybe add docs also?

The previous implementation doesn't match the Rest API documentation: Reset subscription to message position closest to given position. This PR makes it agree on the doc, so I think no more doc is needed?

@sijie sijie added this to the 2.6.0 milestone Feb 10, 2020
@sijie sijie merged commit d2f37a7 into apache:master Feb 10, 2020
tuteng pushed a commit to AmateurEvents/pulsar that referenced this pull request Feb 23, 2020
`ManagedCursorImpl.asyncResetCursor` is used in three kinds of circumstances:
- REST API: create a subscription with messageId. Per the document: Reset subscription to message position closest to given position.
- REST API: reset subscription to a given position: Per the document: Reset subscription to message position closest to given position.
- Consumer seek command.

In all the cases above, when the user provides a MessageId, we should make the best effort to find the closest position, instead of throwing an InvalidCursorPosition Exception. 

This is because if a user provids an invalid position, it's not possible for he or she gets a valid position, since ledger ids for a given topic may not be continuous and only brokers are aware of the order. Therefore, we should avoid throw invalid cursor position but find the nearest position and do the reset stuff.
tuteng pushed a commit to AmateurEvents/pulsar that referenced this pull request Mar 21, 2020
`ManagedCursorImpl.asyncResetCursor` is used in three kinds of circumstances:
- REST API: create a subscription with messageId. Per the document: Reset subscription to message position closest to given position.
- REST API: reset subscription to a given position: Per the document: Reset subscription to message position closest to given position.
- Consumer seek command.

In all the cases above, when the user provides a MessageId, we should make the best effort to find the closest position, instead of throwing an InvalidCursorPosition Exception. 

This is because if a user provids an invalid position, it's not possible for he or she gets a valid position, since ledger ids for a given topic may not be continuous and only brokers are aware of the order. Therefore, we should avoid throw invalid cursor position but find the nearest position and do the reset stuff.

(cherry picked from commit d2f37a7)
tuteng pushed a commit that referenced this pull request Apr 13, 2020
`ManagedCursorImpl.asyncResetCursor` is used in three kinds of circumstances:
- REST API: create a subscription with messageId. Per the document: Reset subscription to message position closest to given position.
- REST API: reset subscription to a given position: Per the document: Reset subscription to message position closest to given position.
- Consumer seek command.

In all the cases above, when the user provides a MessageId, we should make the best effort to find the closest position, instead of throwing an InvalidCursorPosition Exception. 

This is because if a user provids an invalid position, it's not possible for he or she gets a valid position, since ledger ids for a given topic may not be continuous and only brokers are aware of the order. Therefore, we should avoid throw invalid cursor position but find the nearest position and do the reset stuff.

(cherry picked from commit d2f37a7)
jiazhai pushed a commit to jiazhai/pulsar that referenced this pull request May 18, 2020
`ManagedCursorImpl.asyncResetCursor` is used in three kinds of circumstances:
- REST API: create a subscription with messageId. Per the document: Reset subscription to message position closest to given position.
- REST API: reset subscription to a given position: Per the document: Reset subscription to message position closest to given position.
- Consumer seek command.

In all the cases above, when the user provides a MessageId, we should make the best effort to find the closest position, instead of throwing an InvalidCursorPosition Exception.

This is because if a user provids an invalid position, it's not possible for he or she gets a valid position, since ledger ids for a given topic may not be continuous and only brokers are aware of the order. Therefore, we should avoid throw invalid cursor position but find the nearest position and do the reset stuff.
(cherry picked from commit d2f37a7)
huangdx0726 pushed a commit to huangdx0726/pulsar that referenced this pull request Aug 24, 2020
`ManagedCursorImpl.asyncResetCursor` is used in three kinds of circumstances:
- REST API: create a subscription with messageId. Per the document: Reset subscription to message position closest to given position.
- REST API: reset subscription to a given position: Per the document: Reset subscription to message position closest to given position.
- Consumer seek command.

In all the cases above, when the user provides a MessageId, we should make the best effort to find the closest position, instead of throwing an InvalidCursorPosition Exception. 

This is because if a user provids an invalid position, it's not possible for he or she gets a valid position, since ledger ids for a given topic may not be continuous and only brokers are aware of the order. Therefore, we should avoid throw invalid cursor position but find the nearest position and do the reset stuff.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/admin doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. release/2.5.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants