Skip to content

Commit

Permalink
fixups from code review
Browse files Browse the repository at this point in the history
  • Loading branch information
chelseakomlo committed Aug 14, 2017
1 parent c770119 commit 3319fb5
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 58 deletions.
2 changes: 1 addition & 1 deletion api/contexts/contexts.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package contexts

// Context is a type which is searchable via a unique identifier.
// Context defines the scope in which a search for Nomad object operates
type Context string

const (
Expand Down
10 changes: 5 additions & 5 deletions api/search.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
package api

import (
c "github.com/hashicorp/nomad/api/contexts"
"github.com/hashicorp/nomad/api/contexts"
)

type Search struct {
Expand All @@ -15,7 +15,7 @@ func (c *Client) Search() *Search {

// PrefixSearch returns a list of matches for a particular context and prefix. If a
// context is not specified, matches for all contexts are returned.
func (s *Search) PrefixSearch(prefix string, context c.Context) (*SearchResponse, error) {
func (s *Search) PrefixSearch(prefix string, context contexts.Context) (*SearchResponse, error) {
var resp SearchResponse
req := &SearchRequest{Prefix: prefix, Context: context}

Expand All @@ -29,11 +29,11 @@ func (s *Search) PrefixSearch(prefix string, context c.Context) (*SearchResponse

type SearchRequest struct {
Prefix string
Context c.Context
Context contexts.Context
}

type SearchResponse struct {
Matches map[c.Context][]string
Truncations map[c.Context]bool
Matches map[contexts.Context][]string
Truncations map[contexts.Context]bool
QueryMeta
}
22 changes: 11 additions & 11 deletions command/agent/search_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ import (

"github.com/hashicorp/nomad/nomad/mock"
"github.com/hashicorp/nomad/nomad/structs"
a "github.com/stretchr/testify/assert"
"github.com/stretchr/testify/assert"
)

func TestHTTP_SearchWithIllegalMethod(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)
t.Parallel()
httpTest(t, nil, func(s *TestAgent) {
req, err := http.NewRequest("DELETE", "/v1/search", nil)
Expand All @@ -24,7 +24,7 @@ func TestHTTP_SearchWithIllegalMethod(t *testing.T) {
}

func createJobForTest(jobID string, s *TestAgent, t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

job := mock.Job()
job.ID = jobID
Expand All @@ -36,7 +36,7 @@ func createJobForTest(jobID string, s *TestAgent, t *testing.T) {
}

func TestHTTP_Search_POST(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

testJob := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89706"
testJobPrefix := "aaaaaaaa-e8f7-fd38"
Expand Down Expand Up @@ -68,7 +68,7 @@ func TestHTTP_Search_POST(t *testing.T) {
}

func TestHTTP_Search_PUT(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

testJob := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89706"
testJobPrefix := "aaaaaaaa-e8f7-fd38"
Expand Down Expand Up @@ -100,7 +100,7 @@ func TestHTTP_Search_PUT(t *testing.T) {
}

func TestHTTP_Search_MultipleJobs(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

testJobA := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89706"
testJobB := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89707"
Expand Down Expand Up @@ -140,7 +140,7 @@ func TestHTTP_Search_MultipleJobs(t *testing.T) {
}

func TestHTTP_Search_Evaluation(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

t.Parallel()
httpTest(t, nil, func(s *TestAgent) {
Expand Down Expand Up @@ -176,7 +176,7 @@ func TestHTTP_Search_Evaluation(t *testing.T) {
}

func TestHTTP_Search_Allocations(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

t.Parallel()
httpTest(t, nil, func(s *TestAgent) {
Expand Down Expand Up @@ -209,7 +209,7 @@ func TestHTTP_Search_Allocations(t *testing.T) {
}

func TestHTTP_Search_Nodes(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

t.Parallel()
httpTest(t, nil, func(s *TestAgent) {
Expand Down Expand Up @@ -242,7 +242,7 @@ func TestHTTP_Search_Nodes(t *testing.T) {
}

func TestHTTP_Search_NoJob(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

t.Parallel()
httpTest(t, nil, func(s *TestAgent) {
Expand All @@ -265,7 +265,7 @@ func TestHTTP_Search_NoJob(t *testing.T) {
}

func TestHTTP_Search_AllContext(t *testing.T) {
assert := a.New(t)
assert := assert.New(t)

testJobID := "aaaaaaaa-e8f7-fd38-c855-ab94ceb89706"
testJobPrefix := "aaaaaaaa-e8f7-fd38"
Expand Down
2 changes: 1 addition & 1 deletion command/alloc_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"strings"
"time"

humanize "github.com/dustin/go-humanize"
"github.com/dustin/go-humanize"
"github.com/mitchellh/colorstring"

"github.com/hashicorp/nomad/api"
Expand Down
75 changes: 36 additions & 39 deletions nomad/search_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,33 +2,30 @@ package nomad

import (
"fmt"
"strings"

memdb "github.com/hashicorp/go-memdb"
"github.com/hashicorp/nomad/nomad/state"
st "github.com/hashicorp/nomad/nomad/structs"
"github.com/hashicorp/nomad/nomad/structs"
)

// truncateLimit is the maximum number of matches that will be returned for a
// prefix for a specific context
const (
// truncateLimit is the maximum number of matches that will be returned for a
// prefix for a specific context
truncateLimit = 20
)

// allContexts are the available contexts which are searched to find matches
// for a given prefix
var (
allContexts = []st.Context{st.Allocs, st.Jobs, st.Nodes, st.Evals}
// allContexts are the available contexts which are searched to find matches
// for a given prefix
allContexts = []structs.Context{structs.Allocs, structs.Jobs, structs.Nodes, structs.Evals}
)

// Search endpoint is used to look up matches for a given prefix and context
type Search struct {
srv *Server
}

func isSubset(prefix, id string) bool {
return id[0:len(prefix)] == prefix
}

// getMatches extracts matches for an iterator, and returns a list of ids for
// these matches.
func (s *Search) getMatches(iter memdb.ResultIterator, prefix string) ([]string, bool) {
Expand All @@ -42,20 +39,20 @@ func (s *Search) getMatches(iter memdb.ResultIterator, prefix string) ([]string,

var id string
switch t := raw.(type) {
case *st.Job:
id = raw.(*st.Job).ID
case *st.Evaluation:
id = raw.(*st.Evaluation).ID
case *st.Allocation:
id = raw.(*st.Allocation).ID
case *st.Node:
id = raw.(*st.Node).ID
case *structs.Job:
id = raw.(*structs.Job).ID
case *structs.Evaluation:
id = raw.(*structs.Evaluation).ID
case *structs.Allocation:
id = raw.(*structs.Allocation).ID
case *structs.Node:
id = raw.(*structs.Node).ID
default:
s.srv.logger.Printf("[ERR] nomad.resources: unexpected type for resources context: %T", t)
continue
}

if !isSubset(prefix, id) {
if !strings.HasPrefix(id, prefix) {
continue
}

Expand All @@ -67,15 +64,15 @@ func (s *Search) getMatches(iter memdb.ResultIterator, prefix string) ([]string,

// getResourceIter takes a context and returns a memdb iterator specific to
// that context
func getResourceIter(context st.Context, prefix string, ws memdb.WatchSet, state *state.StateStore) (memdb.ResultIterator, error) {
func getResourceIter(context structs.Context, prefix string, ws memdb.WatchSet, state *state.StateStore) (memdb.ResultIterator, error) {
switch context {
case st.Jobs:
case structs.Jobs:
return state.JobsByIDPrefix(ws, prefix)
case st.Evals:
case structs.Evals:
return state.EvalsByIDPrefix(ws, prefix)
case st.Allocs:
case structs.Allocs:
return state.AllocsByIDPrefix(ws, prefix)
case st.Nodes:
case structs.Nodes:
return state.NodesByIDPrefix(ws, prefix)
default:
return nil, fmt.Errorf("context must be one of %v; got %q", allContexts, context)
Expand All @@ -84,8 +81,8 @@ func getResourceIter(context st.Context, prefix string, ws memdb.WatchSet, state

// If the length of a prefix is odd, return a subset to the last even character
// This only applies to UUIDs, jobs are excluded
func roundUUIDDownIfOdd(prefix string, context st.Context) string {
if context == st.Jobs {
func roundUUIDDownIfOdd(prefix string, context structs.Context) string {
if context == structs.Jobs {
return prefix
}

Expand All @@ -98,30 +95,30 @@ func roundUUIDDownIfOdd(prefix string, context st.Context) string {

// PrefixSearch is used to list matches for a given prefix, and returns
// matching jobs, evaluations, allocations, and/or nodes.
func (s *Search) PrefixSearch(args *st.SearchRequest,
reply *st.SearchResponse) error {
reply.Matches = make(map[st.Context][]string)
reply.Truncations = make(map[st.Context]bool)
func (s *Search) PrefixSearch(args *structs.SearchRequest,
reply *structs.SearchResponse) error {
reply.Matches = make(map[structs.Context][]string)
reply.Truncations = make(map[structs.Context]bool)

// Setup the blocking query
opts := blockingOptions{
queryMeta: &reply.QueryMeta,
queryOpts: &st.QueryOptions{},
queryOpts: &structs.QueryOptions{},
run: func(ws memdb.WatchSet, state *state.StateStore) error {

iters := make(map[st.Context]memdb.ResultIterator)
iters := make(map[structs.Context]memdb.ResultIterator)

contexts := allContexts
if args.Context != st.All {
contexts = []st.Context{args.Context}
if args.Context != structs.All {
contexts = []structs.Context{args.Context}
}

for _, e := range contexts {
iter, err := getResourceIter(e, roundUUIDDownIfOdd(args.Prefix, args.Context), ws, state)
for _, ctx := range contexts {
iter, err := getResourceIter(ctx, roundUUIDDownIfOdd(args.Prefix, args.Context), ws, state)
if err != nil {
return err
}
iters[e] = iter
iters[ctx] = iter
}

// Return matches for the given prefix
Expand All @@ -134,8 +131,8 @@ func (s *Search) PrefixSearch(args *st.SearchRequest,
// Set the index for the context. If the context has been specified, it
// will be used as the index of the response. Otherwise, the
// maximum index from all resources will be used.
for _, e := range contexts {
index, err := state.Index(string(e))
for _, ctx := range contexts {
index, err := state.Index(string(ctx))
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion nomad/structs/structs.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ const (
GetterModeDir = "dir"
)

// Context is a type which is searchable via a unique identifier.
// Context defines the scope in which a search for Nomad object operates
type Context string

const (
Expand Down

0 comments on commit 3319fb5

Please sign in to comment.