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

Enable numeric pagination tokens #20299

Merged
merged 4 commits into from
Apr 5, 2024
Merged

Enable numeric pagination tokens #20299

merged 4 commits into from
Apr 5, 2024

Conversation

gulducat
Copy link
Member

@gulducat gulducat commented Apr 4, 2024

For #19339 we will be creating a state index that sorts jobs by ModifyIndex, in order to show most-recently-changed jobs at the top of the Nomad web UI, while continuing to sort appropriately with pagination.

The current pagination logic assumes all NextTokens are strings, which results in odd paging behavior (including infinite loops!) when the token is actually a number, due to lexicographical ordering, e.g. "10" < "2"

This doesn't go as far as fully generalizing the Paginator/Tokenizer relationship (as might be done with comparable or cmp.Ordered generics), but it adds support for uint64 tokens for my immediate use case, and opens a door for further future flexibility.

Copy link
Member

@tgross tgross left a comment

Choose a reason for hiding this comment

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

LGTM! One minor comment about the comment and we're good-to-go.

nomad/state/paginator/paginator.go Outdated Show resolved Hide resolved
@gulducat gulducat force-pushed the uint-pagination-tokens branch from 4c6ac1c to b21f49b Compare April 5, 2024 14:16
@gulducat gulducat merged commit da09778 into main Apr 5, 2024
19 checks passed
@gulducat gulducat deleted the uint-pagination-tokens branch April 5, 2024 14:49
philrenaud pushed a commit that referenced this pull request Apr 18, 2024
* enable uint64 pagination tokens, so they can be compared as numbers instead of strings
* tokenize job ModifyIndex as uint64, so an new upcoming state index can paginate properly
* test require->must
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants