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

makes subscription start from MessageId.latest defaultly #9444

Merged
merged 2 commits into from
Feb 9, 2021

Conversation

aloyszhang
Copy link
Contributor

@aloyszhang aloyszhang commented Feb 3, 2021

Motivation

Currently , if we did not specify the position when create a new subscription, it's will use the earliest position defaulty. https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L2002
This PR makes subscription start from MessageId.latest defaultly

Copy link
Member

@sijie sijie left a comment

Choose a reason for hiding this comment

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

@aloyszhang the change looks good. Can you add a test case?

@sijie sijie added this to the 2.8.0 milestone Feb 3, 2021
@sijie sijie added area/admin release/2.7.1 type/bug The PR fixed a bug or issue reported a bug labels Feb 3, 2021
Copy link
Member

@zymap zymap left a comment

Choose a reason for hiding this comment

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

Hi @aloyszhang, I think the root cause is that we haven't handled the messageId correctly. I left a comment here #9441 (comment).

@aloyszhang
Copy link
Contributor Author

aloyszhang commented Feb 3, 2021

@zymap Yes, you're right, I'll check this later and add some test code.

@aloyszhang
Copy link
Contributor Author

@zymap So far, I didn't find a way to handle a String type parameter like earlist or latest in the broker side. So maybe we have to use specify the messageId like -d '{"ledgerId": 1, "entryId": 1, "partitionIndex": -1}' when using REST API.

Do you have any idea about this?

@aloyszhang
Copy link
Contributor Author

Since this PR does not resolve issue #9441, so remove the Fixes #9441 information.

@aloyszhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

@zymap
Copy link
Member

zymap commented Feb 5, 2021

@aloyszhang I think there has an easier way is add query params at this API and allow it to pass a message position string. If the user only specifies the message position with a string in the query params, we can use it to create a cursor. Otherwise, we can use the current logic to handle it.

For example, if a request URL likes this:

/tenant/ns/topic/subscription/sub-name?position=latest

We can create a subscription from the latest position

if a request URL likes this:

/tenant/ns/topic/subscription/sub-name?position=latest

body: {ledgerId: 1, entryId: 2, batchIndex: 1}

We can use current logic to handle it.

@aloyszhang
Copy link
Contributor Author

aloyszhang commented Feb 5, 2021

@zymap Thanks for your suggestions.

if a request URL likes this:
/tenant/ns/topic/subscription/sub-name?position=latest
body: {ledgerId: 1, entryId: 2, batchIndex: 1}

Only one thing need to be figured out, if the request have boths position and ledgerId:entryId:partitionIndex, I think this may make user more confused about the REST API, right?

@aloyszhang
Copy link
Contributor Author

/pulsarbot run-failure-checks

@sijie
Copy link
Member

sijie commented Feb 8, 2021

@zymap Can you review it again?

@sijie sijie merged commit 28b2094 into apache:master Feb 9, 2021
@codelipenghui codelipenghui added the cherry-picked/branch-2.7 Archived: 2.7 is end of life label Feb 18, 2021
codelipenghui pushed a commit that referenced this pull request Feb 18, 2021
### Motivation
Currently , if we did not specify the position when create a new subscription, it's will use the earliest position defaulty. https://github.com/apache/pulsar/blob/master/pulsar-broker/src/main/java/org/apache/pulsar/broker/admin/impl/PersistentTopicsBase.java#L2002
This PR makes subscription start from MessageId.latest defaultly

(cherry picked from commit 28b2094)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/admin cherry-picked/branch-2.7 Archived: 2.7 is end of life release/2.7.1 type/bug The PR fixed a bug or issue reported a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants