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 2 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
24 changes: 17 additions & 7 deletions api/evaluations_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,17 +65,27 @@ 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]})
// query first page
result, qm, err = e.List(&QueryOptions{
PerPage: int32(1),
})
if err != nil {
t.Fatalf("err: %s", err)
lgfa29 marked this conversation as resolved.
Show resolved Hide resolved
}
if len(result) != 1 {
t.Fatalf("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,
})
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])
t.Fatalf("expected no evals after last one but got %d: %#v", len(result), result)
}

// Query evaluations using a filter.
Expand Down
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
59 changes: 47 additions & 12 deletions nomad/deployment_endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1271,11 +1271,18 @@ func TestDeploymentEndpoint_List_Pagination(t *testing.T) {
{id: "aaaaaabb-3350-4b4b-d185-0e1992ed43e9"}, // 4
{id: "aaaaaacc-3350-4b4b-d185-0e1992ed43e9"}, // 5
{id: "aaaaaadd-3350-4b4b-d185-0e1992ed43e9"}, // 6
{id: "00000111-3350-4b4b-d185-0e1992ed43e9"}, // 7
{}, // 8, index missing
{id: "bbbb1111-3350-4b4b-d185-0e1992ed43e9"}, // 9
}

state := s1.fsm.State()

for i, m := range mocks {
if m.id == "" {
continue
}

index := 1000 + uint64(i)
deployment := mock.Deployment()
deployment.Status = structs.DeploymentStatusCancelled
Expand Down Expand Up @@ -1305,7 +1312,7 @@ func TestDeploymentEndpoint_List_Pagination(t *testing.T) {
{
name: "test01 size-2 page-1 default NS",
pageSize: 2,
expectedNextToken: "aaaaaaaa-3350-4b4b-d185-0e1992ed43e9",
expectedNextToken: "1003-aaaaaaaa-3350-4b4b-d185-0e1992ed43e9",
expectedIDs: []string{
"aaaa1111-3350-4b4b-d185-0e1992ed43e9",
"aaaaaa22-3350-4b4b-d185-0e1992ed43e9",
Expand All @@ -1315,7 +1322,7 @@ func TestDeploymentEndpoint_List_Pagination(t *testing.T) {
name: "test02 size-2 page-1 default NS with prefix",
prefix: "aaaa",
pageSize: 2,
expectedNextToken: "aaaaaaaa-3350-4b4b-d185-0e1992ed43e9",
expectedNextToken: "aaaaaaaa-3350-4b4b-d185-0e1992ed43e9", // prefix results are not sorted by create index
expectedIDs: []string{
"aaaa1111-3350-4b4b-d185-0e1992ed43e9",
"aaaaaa22-3350-4b4b-d185-0e1992ed43e9",
Expand All @@ -1324,8 +1331,8 @@ func TestDeploymentEndpoint_List_Pagination(t *testing.T) {
{
name: "test03 size-2 page-2 default NS",
pageSize: 2,
nextToken: "aaaaaaaa-3350-4b4b-d185-0e1992ed43e9",
expectedNextToken: "aaaaaacc-3350-4b4b-d185-0e1992ed43e9",
nextToken: "1003-aaaaaaaa-3350-4b4b-d185-0e1992ed43e9",
expectedNextToken: "1005-aaaaaacc-3350-4b4b-d185-0e1992ed43e9",
expectedIDs: []string{
"aaaaaaaa-3350-4b4b-d185-0e1992ed43e9",
"aaaaaabb-3350-4b4b-d185-0e1992ed43e9",
Expand All @@ -1343,14 +1350,25 @@ func TestDeploymentEndpoint_List_Pagination(t *testing.T) {
},
},
{
name: "test05 no valid results with filters and prefix",
name: "test05 size-2 page-2 all namespaces",
namespace: "*",
pageSize: 2,
nextToken: "1002-aaaaaa33-3350-4b4b-d185-0e1992ed43e9",
expectedNextToken: "1004-aaaaaabb-3350-4b4b-d185-0e1992ed43e9",
expectedIDs: []string{
"aaaaaa33-3350-4b4b-d185-0e1992ed43e9",
"aaaaaaaa-3350-4b4b-d185-0e1992ed43e9",
},
},
{
name: "test06 no valid results with filters and prefix",
prefix: "cccc",
pageSize: 2,
nextToken: "",
expectedIDs: []string{},
},
{
name: "test06 go-bexpr filter",
name: "test07 go-bexpr filter",
namespace: "*",
filter: `ID matches "^a+[123]"`,
expectedIDs: []string{
Expand All @@ -1360,40 +1378,57 @@ func TestDeploymentEndpoint_List_Pagination(t *testing.T) {
},
},
{
name: "test07 go-bexpr filter with pagination",
name: "test08 go-bexpr filter with pagination",
namespace: "*",
filter: `ID matches "^a+[123]"`,
pageSize: 2,
expectedNextToken: "aaaaaa33-3350-4b4b-d185-0e1992ed43e9",
expectedNextToken: "1002-aaaaaa33-3350-4b4b-d185-0e1992ed43e9",
expectedIDs: []string{
"aaaa1111-3350-4b4b-d185-0e1992ed43e9",
"aaaaaa22-3350-4b4b-d185-0e1992ed43e9",
},
},
{
name: "test08 go-bexpr filter in namespace",
name: "test09 go-bexpr filter in namespace",
namespace: "non-default",
filter: `Status == "cancelled"`,
expectedIDs: []string{
"aaaaaa33-3350-4b4b-d185-0e1992ed43e9",
},
},
{
name: "test09 go-bexpr wrong namespace",
name: "test10 go-bexpr wrong namespace",
namespace: "default",
filter: `Namespace == "non-default"`,
expectedIDs: []string{},
},
{
name: "test10 go-bexpr invalid expression",
name: "test11 go-bexpr invalid expression",
filter: `NotValid`,
expectedError: "failed to read filter expression",
},
{
name: "test11 go-bexpr invalid field",
name: "test12 go-bexpr invalid field",
filter: `InvalidField == "value"`,
expectedError: "error finding value in datum",
},
{
name: "test13 non-lexicographic order",
pageSize: 1,
nextToken: "1007-00000111-3350-4b4b-d185-0e1992ed43e9",
expectedNextToken: "1009-bbbb1111-3350-4b4b-d185-0e1992ed43e9",
expectedIDs: []string{
"00000111-3350-4b4b-d185-0e1992ed43e9",
},
},
{
name: "test14 missing index",
pageSize: 1,
nextToken: "1008-e9522802-0cd8-4b1d-9c9e-ab3d97938371",
expectedIDs: []string{
"bbbb1111-3350-4b4b-d185-0e1992ed43e9",
},
},
}

for _, tc := range cases {
Expand Down
31 changes: 30 additions & 1 deletion nomad/eval_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,30 @@ const (
DefaultDequeueTimeout = time.Second
)

// EvalPaginationIterator is a wrapper over a go-memdb iterator that implements
// the paginator Iterator interface.
type EvalPaginationIterator struct {
iter memdb.ResultIterator
byCreateIndex bool
}

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

eval := raw.(*structs.Evaluation)
token := eval.ID

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

return token, eval
}

// Eval endpoint is used for eval interactions
type Eval struct {
srv *Server
Expand Down Expand Up @@ -414,13 +438,17 @@ func (e *Eval) List(args *structs.EvalListRequest, reply *structs.EvalListRespon
// Scan all the evaluations
var err error
var iter memdb.ResultIterator
var evalIter EvalPaginationIterator

if prefix := args.QueryOptions.Prefix; prefix != "" {
iter, err = store.EvalsByIDPrefix(ws, namespace, prefix)
evalIter.byCreateIndex = false
} else if namespace != structs.AllNamespacesSentinel {
iter, err = store.EvalsByNamespaceOrdered(ws, namespace, args.Ascending)
evalIter.byCreateIndex = true
} else {
iter, err = store.Evals(ws, args.Ascending)
evalIter.byCreateIndex = true
}
if err != nil {
return err
Expand All @@ -432,9 +460,10 @@ func (e *Eval) List(args *structs.EvalListRequest, reply *structs.EvalListRespon
}
return false
})
evalIter.iter = iter

var evals []*structs.Evaluation
paginator, err := state.NewPaginator(iter, args.QueryOptions,
paginator, err := state.NewPaginator(evalIter, args.QueryOptions,
func(raw interface{}) error {
eval := raw.(*structs.Evaluation)
evals = append(evals, eval)
Expand Down
Loading