-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
evaluations list pagination and filtering #11648
Conversation
87b5444
to
f589249
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
Do you think we're at the point where it would be worth documenting the general flow of pagination in https://www.nomadproject.io/api-docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one copy-paste variable naming issue. Made some observations on the filtering method itself around readability and comments.
After reading @DerekStrickland's comments I went over the code again to use the This is the sample I used, but a test in package main
import (
"fmt"
"github.com/hashicorp/nomad/api"
)
func main() {
c, _ := api.NewClient(api.DefaultConfig())
eval := c.Evaluations()
lastToken := ""
for {
q := &api.QueryOptions{
LastToken: lastToken,
PerPage: 1,
}
l, qm, _ := eval.List(q)
fmt.Printf("%v\n", l[0].ID)
lastToken = qm.LastToken
if qm.LastToken == "" {
break
}
}
} |
Good catch. That also means we have a bug in the existing |
I definitely considered that but I figured I'd do that once we had a second example? |
API queries can request pagination using the `NextToken` and `PerPage` fields of `QueryOptions`, when supported by the underlying API. Add a `NextToken` field to the `structs.QueryMeta` so that we have a common field across RPCs to tell the caller where to resume paging from on their next API call. Include this field on the `api.QueryMeta` as well so that it's available for future versions of List HTTP APIs that wrap the response with `QueryMeta` rather than returning a simple list of structs. In the meantime callers can get the `X-Nomad-NextToken`.
346e6e7
to
4212a20
Compare
Add pagination to the `Eval.List` RPC by checking for pagination token and page size in `QueryOptions`. This will allow resuming from the last ID seen so long as the query parameters and the state store itself are unchanged between requests. Add filtering by job ID or evaluation status over the results we get out of the state store.
4212a20
to
86459ca
Compare
Parse the query parameters of the `Eval.List` API into the arguments expected for filtering in the RPC call.
86459ca
to
8560246
Compare
Ok @DerekStrickland @lgfa29 this should be ready for re-review. Thanks for sticking this one out... it's been a slog 😁 |
Refactor to expose only a Page method that populates the result set by repeatedly calling an append function over the paged iterator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -69,8 +69,10 @@ type QueryOptions struct { | |||
// paginated lists. | |||
PerPage int32 | |||
|
|||
// NextToken is the token used indicate where to start paging for queries | |||
// that support paginated lists. | |||
// NextToken is the token used to indicate where to start paging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It dawned on me last night, that we might need to keep PreviousToken too. This is not a blocker. Just a thought for conversation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't actually page backwards with the pager though -- you just call it with whatever the previous "next token" was (or with no token for the first page). So for now I'm going to call YAGNI on that and we can see if callers end up really needing it. 😀
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. |
Fixes #11621
Add pagination to the
Eval.List
RPC by checking for pagination tokenand page size in
QueryOptions
. This will allow resuming from thelast ID seen so long as the query parameters and the state store
itself are unchanged between requests.
Add filtering by job ID or evaluation status over the results we get
out of the state store.
Add a
NextToken
field to thestructs.QueryMeta
so that we have acommon field across RPCs to tell the caller where to resume paging
from on their next API call. Include this field on the
api.QueryMeta
as well so that it's available for future versions of List HTTP APIs
that wrap the response with
QueryMeta
rather than returning a simplelist of structs.
Note to reviewers: this is probably best reviewed commit-by-commit. It's a large PR but most of the LoC is tests. @ChaiWithJai I've added you as a reviewer mostly so you can see the implications for how the UI will need to do paging.