api: return sorted results in certain list endpoints #12054
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shoenig
force-pushed
the
b-creation-indexes
branch
from
2ceea96 to
8e0202c
Compare
shoenig
force-pushed
the
b-creation-indexes
branch
from
24f3308 to
54e19b4
Compare
shoenig
force-pushed
the
b-creation-indexes
branch
from
54e19b4 to
21b0a40
Compare
shoenig
force-pushed
the
b-creation-indexes
branch
from
21b0a40 to
a2a6148
Compare
shoenig
force-pushed
the
b-creation-indexes
branch
from
a2a6148 to
065dba1
Compare
shoenig
force-pushed
the
b-creation-indexes
branch
from
065dba1 to
a5727ed
Compare
shoenig
force-pushed
the
b-creation-indexes
branch
from
a5727ed to
0d4783b
Compare
shoenig
force-pushed
the
b-creation-indexes
branch
from
0d4783b to
aaa0ccb
Compare
shoenig
commented
Feb 14, 2022
nomad/deployment_endpoint.go
Outdated
| return args.ShouldBeFiltered(deployment) | ||
| } | ||
| return false | ||
| }) |
There was a problem hiding this comment.
Because we now rely on a _prefix iterator over namespaces, we need to filter out any deployments that erroneously match the prefix. (e.g. want foo, but also matches fooo).
There was a problem hiding this comment.
Maybe it would be worth having a test for this? Just to make sure the behaviour or ShouldBeFiltered changes unexpectedly.
lgfa29
approved these changes
Feb 15, 2022
nomad/structs/structs.go
Outdated
| @@ -859,6 +859,8 @@ type EvalDequeueRequest struct { | |||
| type EvalListRequest struct { | |||
| FilterJobID string | |||
| FilterEvalStatus string | |||
| FilterNamespace string | |||
There was a problem hiding this comment.
Do we need a new namespace filter? What's the difference from QueryOptions.Namespace?
nomad/deployment_endpoint.go
Outdated
| return args.ShouldBeFiltered(deployment) | ||
| } | ||
| return false | ||
| }) |
There was a problem hiding this comment.
Maybe it would be worth having a test for this? Just to make sure the behaviour or ShouldBeFiltered changes unexpectedly.
These API endpoints now return results in chronological order. They can return results in reverse chronological order by setting the query parameter ascending=true. - Eval.List - Deployment.List
shoenig
force-pushed
the
b-creation-indexes
branch
from
aaa0ccb to
b432f37
Compare
lgfa29
added a commit
that referenced
this pull request
Feb 24, 2022
The paginator logic was built when go-memdb iterators would return items ordered lexicographically by their prefixes, but #12054 added the option for some tables to return results ordered by their `CreateIndex` instead, which invalidated the previous paginator assumption. This fix changes the paginator so that it skips values that are different from the token bein searched instead of just smaller ones.
lgfa29
added a commit
that referenced
this pull request
Feb 28, 2022
The paginator logic was built when go-memdb iterators would return items ordered lexicographically by their ID prefixes, but #12054 added the option for some tables to return results ordered by their `CreateIndex` instead, which invalidated the previous paginator assumption. The iterator used for pagination must still return results in some order so that the paginator can properly handle requests where the next_token value is not present in the results anymore (e.g., the eval was GC'ed). In these situations, the paginator will start the returned page in the first element right after where the requested token should've been. This commit moves the logic to generate pagination tokens from the elements being paginated to the iterator itself so that callers can have more control over the token format to make sure they are properly ordered and stable. It also allows configuring the paginator as being ordered in ascending or descending order, which is relevant when looking for a token that may not be present anymore.
lgfa29
added a commit
that referenced
this pull request
Mar 1, 2022
The paginator logic was built when go-memdb iterators would return items ordered lexicographically by their ID prefixes, but #12054 added the option for some tables to return results ordered by their `CreateIndex` instead, which invalidated the previous paginator assumption. The iterator used for pagination must still return results in some order so that the paginator can properly handle requests where the next_token value is not present in the results anymore (e.g., the eval was GC'ed). In these situations, the paginator will start the returned page in the first element right after where the requested token should've been. This commit moves the logic to generate pagination tokens from the elements being paginated to the iterator itself so that callers can have more control over the token format to make sure they are properly ordered and stable. It also allows configuring the paginator as being ordered in ascending or descending order, which is relevant when looking for a token that may not be present anymore.
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These API endpoints now return results in chronological order. They
can return results in reverse chronological order by setting the
query parameter ascending=true.
We can add more endpoints following the same pattern. But lets get these out of the way first and see if what we're doing here actually makes sense / is necessary in light of
go-bexpr.Parts of #11742