api: paginated results with different ordering

[lgfa29]

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.

---

Note to reviewers: this PR was re-worked a few times, so I squashed and force-pused a single commit because the commit history was a mess. The resulting changes turned out to be not that big, but let me know if it would help to break it into multiple commits instead of just one.

[tgross]

What happens if the X-Nomad-NextToken is no longer valid at the time we make this call? Don't we end up skipping all the way to the end?

[lgfa29]

Hum...that's right. I may have to go with my plan B then, which was to make GetID configurable to return either ID or CreateIndex. It does create a weird API though, because you sometimes paginate with an UUID and other times with a Raft index.

Back to the drawing board 😅

[tgross]

Yeah I think it's the same problem either way, just sorting lexicographically vs numerically.

[lgfa29]

Deployment.GetID is actually used for other things, so we can't remove it.

[tgross]

LGTM! I've left some comments but nothing blocking.

[tgross]

API musing, feel free to ignore:

I don't love an API where we have to wrap the go-memdb iterator for every struct we want to paginate. But the only alternative that I can see is a new PageTokenizer interface implemented by Deployment, Evaluation, etc. And then Paginator could expect all the ResultIterators return results that are PageTokenizers. So that's probably not a good path to go down?

Or maybe Paginator could take a ResultIterator and a function to get the token (it'd close over byCreateIndex)? That'd be called something like:

paginator, err := state.NewPaginator(iter, args.QueryOptions,
	func(raw interface{}) error {
		deploy := raw.(*structs.Deployment)
		deploys = append(deploys, deploy)
		return nil
	},
	func(raw interface{}) string {
		deploy := raw.(*structs.Deployment)
		token := deploy.ID
		if byCreateIndex {
				token = fmt.Sprintf("%v-%v", deploy.CreateIndex, deploy.ID)
		}
	},
)

But maybe having two function parameters starts calling into question whether Paginator should be generic over the type intended to be returned by the ResultIterator. (Oh no, the G-word! 😀 )

[shoenig]

Yeah actually I think generics could clean this up quite a bit. Maybe 1.4 hackathon 🥳

[lgfa29]

a new PageTokenizer interface implemented by Deployment, Evaluation, etc. And then Paginator could expect all the ResultIterators return results that are PageTokenizers.

This is similar to the approach we had before, with the GetID right? The problem I found is that the struct doesn't know about the request details, so it can't tell if it should ID or CreateIndex unless we add a new field, which sounded not great.

Or maybe Paginator could take a ResultIterator and a function to get the token

This was also something that I experimented with, but it felt like I was writing JavasScript 😅

Another interface I tried was to have a separate method in the Iterator, so like:

type Iterator interface {
	Next() interface{}
        GetToken(interface{}) string
}

But that didn't help much because you would still need to wrap the ResultIterator.

Maybe we can do it with two interfaces? If the iterator implements an interface with GetToken we would use that, otherwise we use GetID from the raw element like before.

[tgross]

The problem I found is that the struct doesn't know about the request details, so it can't tell if it should ID or CreateIndex unless we add a new field, which sounded not great.

That's a good point. We'd have to thread the CreateIndex through somehow, which ends up being the same as the "pass a function to get the token" implementation.

This was also something that I experimented with, but it felt like I was writing JavasScript 😅

😆

If the iterator implements an interface

I don't know if you can actually check that without reflection or calling a possibly non-existent method?

In any case, I think what we've got here will get the job done. We can always come back and refine it as we have more paginated API examples. (Ex. if we end up wanting to paginate an API by neither ID nor CreateIndex)

[lgfa29]

In any case, I think what we've got here will get the job done. We can always come back and refine it as we have more paginated API examples. (Ex. if we end up wanting to paginate an API by neither ID nor CreateIndex)

+1. The two interfaces worked, but it was weird to have two very different ways to integrate with the Paginator.

[shoenig]

LGTM

Don't forget we also need to stack #12090 on top of this

[shoenig]

Yeah actually I think generics could clean this up quite a bit. Maybe 1.4 hackathon 🥳

[github-actions]

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.