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.

Sign up for GitHub

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

api: paginated results with different ordering #12128

Merged
merged 3 commits into from
Mar 1, 2022
Merged

api: paginated results with different ordering #12128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5942f32
api: paginated results with different ordering
lgfa29 Feb 24, 2022
4b7d8d1
fix api package tests
lgfa29 Mar 1, 2022
ab2a6e1
tests: migrate api/evaluations_test.go to testity
lgfa29 Mar 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
119 changes: 39 additions & 80 deletions api/evaluations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
package api

import (
"reflect"
"sort"
"strings"
"testing"

"github.com/hashicorp/nomad/api/internal/testutil"
"github.com/stretchr/testify/require"
)

func TestEvaluations_List(t *testing.T) {
Expand All @@ -17,41 +16,27 @@ func TestEvaluations_List(t *testing.T) {

// Listing when nothing exists returns empty
result, qm, err := e.List(nil)
if err != nil {
t.Fatalf("err: %s", err)
}
if qm.LastIndex != 0 {
t.Fatalf("bad index: %d", qm.LastIndex)
}
if n := len(result); n != 0 {
t.Fatalf("expected 0 evaluations, got: %d", n)
}
require.NoError(t, err)
require.Equal(t, uint64(0), qm.LastIndex, "bad index")
require.Equal(t, 0, len(result), "expected 0 evaluations")

// Register a job. This will create an evaluation.
jobs := c.Jobs()
job := testJob()
resp, wm, err := jobs.Register(job, nil)
if err != nil {
t.Fatalf("err: %s", err)
}
require.NoError(t, err)
assertWriteMeta(t, wm)

// Check the evaluations again
result, qm, err = e.List(nil)
if err != nil {
t.Fatalf("err: %s", err)
}
require.NoError(t, err)
assertQueryMeta(t, qm)

// if the eval fails fast there can be more than 1
// but they are in order of most recent first, so look at the last one
if len(result) == 0 {
t.Fatalf("expected eval (%s), got none", resp.EvalID)
}
require.Greater(t, len(result), 0, "expected eval (%s), got none", resp.EvalID)
idx := len(result) - 1
if result[idx].ID != resp.EvalID {
t.Fatalf("expected eval (%s), got: %#v", resp.EvalID, result[idx])
}
require.Equal(t, resp.EvalID, result[idx].ID, "expected eval (%s), got: %#v", resp.EvalID, result[idx])

// wait until the 2nd eval shows up before we try paging
results := []*Evaluation{}
Expand All @@ -65,26 +50,26 @@ func TestEvaluations_List(t *testing.T) {
t.Fatalf("err: %s", err)
})

// Check the evaluations again with paging; note that while this
// package sorts by timestamp, the actual HTTP API sorts by ID
// so we need to use that for the NextToken
ids := []string{results[0].ID, results[1].ID}
sort.Strings(ids)
result, qm, err = e.List(&QueryOptions{PerPage: int32(1), NextToken: ids[1]})
if err != nil {
t.Fatalf("err: %s", err)
}
if len(result) != 1 {
t.Fatalf("expected no evals after last one but got %v", result[0])
}
// query first page
result, qm, err = e.List(&QueryOptions{
PerPage: int32(1),
})
require.NoError(t, err)
require.Equal(t, 1, len(result), "expected no evals after last one but got %d: %#v", len(result), result)

// query second page
result, qm, err = e.List(&QueryOptions{
PerPage: int32(1),
NextToken: qm.NextToken,
})
require.NoError(t, err)
require.Equal(t, 1, len(result), "expected no evals after last one but got %d: %#v", len(result), result)

// Query evaluations using a filter.
results, _, err = e.List(&QueryOptions{
Filter: `TriggeredBy == "job-register"`,
})
if len(result) != 1 {
t.Fatalf("expected 1 eval, got %d", len(result))
}
require.Equal(t, 1, len(result), "expected 1 eval, got %d", len(result))
}

func TestEvaluations_PrefixList(t *testing.T) {
Expand All @@ -95,36 +80,25 @@ func TestEvaluations_PrefixList(t *testing.T) {

// Listing when nothing exists returns empty
result, qm, err := e.PrefixList("abcdef")
if err != nil {
t.Fatalf("err: %s", err)
}
if qm.LastIndex != 0 {
t.Fatalf("bad index: %d", qm.LastIndex)
}
if n := len(result); n != 0 {
t.Fatalf("expected 0 evaluations, got: %d", n)
}
require.NoError(t, err)
require.Equal(t, uint64(0), qm.LastIndex, "bad index")
require.Equal(t, 0, len(result), "expected 0 evaluations")

// Register a job. This will create an evaluation.
jobs := c.Jobs()
job := testJob()
resp, wm, err := jobs.Register(job, nil)
if err != nil {
t.Fatalf("err: %s", err)
}
require.NoError(t, err)
assertWriteMeta(t, wm)

// Check the evaluations again
result, qm, err = e.PrefixList(resp.EvalID[:4])
if err != nil {
t.Fatalf("err: %s", err)
}
require.NoError(t, err)
assertQueryMeta(t, qm)

// Check if we have the right list
if len(result) != 1 || result[0].ID != resp.EvalID {
t.Fatalf("bad: %#v", result)
}
require.Equal(t, 1, len(result))
require.Equal(t, resp.EvalID, result[0].ID)
}

func TestEvaluations_Info(t *testing.T) {
Expand All @@ -135,30 +109,23 @@ func TestEvaluations_Info(t *testing.T) {

// Querying a nonexistent evaluation returns error
_, _, err := e.Info("8E231CF4-CA48-43FF-B694-5801E69E22FA", nil)
if err == nil || !strings.Contains(err.Error(), "not found") {
t.Fatalf("expected not found error, got: %s", err)
}
require.Error(t, err)

// Register a job. Creates a new evaluation.
jobs := c.Jobs()
job := testJob()
resp, wm, err := jobs.Register(job, nil)
if err != nil {
t.Fatalf("err: %s", err)
}
require.NoError(t, err)
assertWriteMeta(t, wm)

// Try looking up by the new eval ID
result, qm, err := e.Info(resp.EvalID, nil)
if err != nil {
t.Fatalf("err: %s", err)
}
require.NoError(t, err)
assertQueryMeta(t, qm)

// Check that we got the right result
if result == nil || result.ID != resp.EvalID {
t.Fatalf("expected eval %q, got: %#v", resp.EvalID, result)
}
require.NotNil(t, result)
require.Equal(t, resp.EvalID, result.ID)
}

func TestEvaluations_Allocations(t *testing.T) {
Expand All @@ -169,15 +136,9 @@ func TestEvaluations_Allocations(t *testing.T) {

// Returns empty if no allocations
allocs, qm, err := e.Allocations("8E231CF4-CA48-43FF-B694-5801E69E22FA", nil)
if err != nil {
t.Fatalf("err: %s", err)
}
if qm.LastIndex != 0 {
t.Fatalf("bad index: %d", qm.LastIndex)
}
if n := len(allocs); n != 0 {
t.Fatalf("expected 0 allocs, got: %d", n)
}
require.NoError(t, err)
require.Equal(t, uint64(0), qm.LastIndex, "bad index")
require.Equal(t, 0, len(allocs), "expected 0 evaluations")
}

func TestEvaluations_Sort(t *testing.T) {
Expand All @@ -194,7 +155,5 @@ func TestEvaluations_Sort(t *testing.T) {
{CreateIndex: 2},
{CreateIndex: 1},
}
if !reflect.DeepEqual(evals, expect) {
t.Fatalf("\n\n%#v\n\n%#v", evals, expect)
}
require.Equal(t, expect, evals)
}
32 changes: 31 additions & 1 deletion nomad/deployment_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,30 @@ import (
"github.com/hashicorp/nomad/nomad/structs"
)

// DeploymentPaginationIterator is a wrapper over a go-memdb iterator that
Copy link
Member

@tgross tgross Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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! 😀 )

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@lgfa29 lgfa29 Mar 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

// implements the paginator Iterator interface.
type DeploymentPaginationIterator struct {
iter memdb.ResultIterator
byCreateIndex bool
}

func (it DeploymentPaginationIterator) Next() (string, interface{}) {
raw := it.iter.Next()
if raw == nil {
return "", nil
}

d := raw.(*structs.Deployment)
token := d.ID

// prefix the pagination token by CreateIndex to keep it properly sorted.
if it.byCreateIndex {
token = fmt.Sprintf("%v-%v", d.CreateIndex, d.ID)
}

return token, d
}

// Deployment endpoint is used for manipulating deployments
type Deployment struct {
srv *Server
Expand Down Expand Up @@ -409,20 +433,26 @@ func (d *Deployment) List(args *structs.DeploymentListRequest, reply *structs.De
// Capture all the deployments
var err error
var iter memdb.ResultIterator
var deploymentIter DeploymentPaginationIterator

if prefix := args.QueryOptions.Prefix; prefix != "" {
iter, err = store.DeploymentsByIDPrefix(ws, namespace, prefix)
deploymentIter.byCreateIndex = false
} else if namespace != structs.AllNamespacesSentinel {
iter, err = store.DeploymentsByNamespaceOrdered(ws, namespace, args.Ascending)
deploymentIter.byCreateIndex = true
} else {
iter, err = store.Deployments(ws, args.Ascending)
deploymentIter.byCreateIndex = true
}
if err != nil {
return err
}

deploymentIter.iter = iter

var deploys []*structs.Deployment
paginator, err := state.NewPaginator(iter, args.QueryOptions,
paginator, err := state.NewPaginator(deploymentIter, args.QueryOptions,
func(raw interface{}) error {
deploy := raw.(*structs.Deployment)
deploys = append(deploys, deploy)
Expand Down
Loading