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

Scan cursor has unreasonable config #3421

Closed
Nicole00 opened this issue Dec 7, 2021 · 9 comments · Fixed by #3427
Closed

Scan cursor has unreasonable config #3421

Nicole00 opened this issue Dec 7, 2021 · 9 comments · Fixed by #3427
Assignees
Labels
type/bug Type: something is unexpected
Milestone

Comments

@Nicole00
Copy link
Contributor

Nicole00 commented Dec 7, 2021

this issue appear after #3262.

  1. The scan request requires the client to explicitly set the has_next in the request ScanCursor to true, which is unreasonable. The value of has_next should not allow the client to parameterize.

  2. When client call scan interface for the first time, client needs to set next_cursor to "".getBytes to scan from the beginning. I suggest to change "".getBytes to null, then clients will not need to explicitly set the next_cursor.

@Nicole00 Nicole00 added type/bug Type: something is unexpected need to discuss Solution: issue or PR without a clear conclusion on whether to handle it labels Dec 7, 2021
@Sophie-Xie Sophie-Xie added this to the v3.0.0 milestone Dec 7, 2021
@Shylock-Hg
Copy link
Contributor

You could set has_next to false.

@Nicole00
Copy link
Contributor Author

Nicole00 commented Dec 7, 2021

You could set has_next to false.

What's the meaning for client to set has_next to false.

@Shylock-Hg
Copy link
Contributor

You could set has_next to false.

What's the meaning for client to set has_next to false.

No cursor.

@Nicole00
Copy link
Contributor Author

Nicole00 commented Dec 7, 2021

You could set has_next to false.

What's the meaning for client to set has_next to false.

No cursor.

null scanCurosr can mean no cursor, why need client to explicitly set has_next.

@Shylock-Hg
Copy link
Contributor

You could set has_next to false.

What's the meaning for client to set has_next to false.

No cursor.

null scanCurosr can mean no cursor, why need client to explicitly set has_next.

Just follow the origin interface.

@Nicole00
Copy link
Contributor Author

Nicole00 commented Dec 7, 2021

You could set has_next to false.

What's the meaning for client to set has_next to false.

No cursor.

null scanCurosr can mean no cursor, why need client to explicitly set has_next.

Just follow the origin interface.

But in the origin interface, client does not need to set has_next.

@Shylock-Hg
Copy link
Contributor

You could set has_next to false.

What's the meaning for client to set has_next to false.

No cursor.

null scanCurosr can mean no cursor, why need client to explicitly set has_next.

Just follow the origin interface.

But in the origin interface, client does not need to set has_next.

Ok, it's my fault. Now we should set it in request.

@jievince
Copy link
Contributor

jievince commented Dec 7, 2021

auto ret = plan.go(partId, cursor.get_has_next() ? *cursor.get_next_cursor() : "");

If has_next is true but next_cursor is not set, it'll cause storaged to crash. So you always need to check if next_cursor is set.

@Shylock-Hg
Copy link
Contributor

auto ret = plan.go(partId, cursor.get_has_next() ? *cursor.get_next_cursor() : "");

If has_next is true but next_cursor is not set, it'll cause storaged to crash. So you always need to check if next_cursor is set.

Yes, maybe we could remove has_next

@Shylock-Hg Shylock-Hg reopened this Dec 7, 2021
@Shylock-Hg Shylock-Hg mentioned this issue Dec 8, 2021
7 tasks
@Sophie-Xie Sophie-Xie removed the need to discuss Solution: issue or PR without a clear conclusion on whether to handle it label Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Type: something is unexpected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants