autocomplete api #2964

chelseakomlo · 2017-08-04T15:26:05Z

Adds an HTTP api which acceps a prefix and a context, which can be jobs, evaluations, allocations, or nodes. If a context is not set, all types are returned that match the given prefix.

This will be used to extend autocomplete functionality for the CLI.

requires further refactoring and logic for more contexts

makes context singular

adding nodes to resources list endpoint

set the index for a resources response

add test for when no prefixes are matched add test for no context at HTTP api

extract magic number into variable

add tests for edge cases

dadgar · 2017-08-04T16:59:07Z

nomad/resources_endpoint.go

+			iters := make(map[string]memdb.ResultIterator)
+
+			if args.Context != "" {
+				iter, err := getResourceIter(args.Context, args.Prefix, ws, state)


// Top of file var ( allContexts = []string{"allocs", "nodes", "jobs", "evals"} )

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

dadgar · 2017-08-04T17:01:40Z

nomad/resources_endpoint.go

+		case *structs.Node:
+			id = raw.(*structs.Node).ID
+		default:
+			continue


Log an error here

dadgar · 2017-08-04T17:04:06Z

nomad/resources_endpoint.go

+			// is the only non-empty match set, and the index is set for it.
+			// If the context was not specified, we set the index of the first
+			// non-empty match set.
+			for k, v := range reply.Matches {


I would go through each context and call state.Index on the context and then take the Max of all the returned indexes.

dadgar · 2017-08-04T17:04:38Z

nomad/resources_endpoint.go

+	case "nodes":
+		return state.NodesByIDPrefix(ws, prefix)
+	default:
+		return nil, fmt.Errorf("invalid context")


fmt.Errorf("context must be one of %v; got %q", allContexts, context)

dadgar · 2017-08-04T17:06:12Z

nomad/structs/structs.go

@@ -231,6 +231,22 @@ type NodeSpecificRequest struct {
 	QueryOptions
 }

+// ResourcesResponse is used to return matches and information about whether
+// the match list is truncated specific to each type of context.
+type ResourcesResponse struct {


ResourceListResponse

dadgar · 2017-08-04T17:06:43Z

nomad/structs/structs.go

+// ResourcesResponse is used to return matches and information about whether
+// the match list is truncated specific to each type of context.
+type ResourcesResponse struct {
+	Matches     map[string][]string


Put a comment please

dadgar · 2017-08-04T17:07:53Z

command/agent/resources_endpoint_test.go

+		respW := httptest.NewRecorder()
+
+		resp, err := s.Server.ResourcesRequest(respW, req)
+		if err != nil {


dadgar · 2017-08-04T17:08:09Z

command/agent/resources_endpoint_test.go

+
+		res := resp.(structs.ResourcesResponse)
+
+		assert.Equal(t, 1, len(res.Matches))


assert := assert.New(t) and then use that

dadgar · 2017-08-04T17:09:05Z

command/agent/resources_endpoint_test.go

+	})
+}
+
+func TestHTTP_Resources_NoContext(t *testing.T) {


Should probably test more than just the job is returned

dadgar · 2017-08-04T17:09:22Z

command/agent/resources_endpoint_test.go

+	})
+}
+
+func TestHTTP_ResoucesList_Evaluation(t *testing.T) {


Allocs and nodes?

add http tests for remaining contexts

dadgar · 2017-08-04T23:17:47Z

nomad/resources_endpoint.go

+		case *structs.Node:
+			id = raw.(*structs.Node).ID
+		default:
+			//s.logger.Printf("[ERR] nomad: unexpected type for resources context; not a job, allocation, node, or evaluation")


You can either make it a method of Resources or have it return an error. Also we should just use the variable so that if contexts get added it will automatically propagate.

[ERR] nomad.resources:

dadgar · 2017-08-04T23:22:15Z

nomad/resources_endpoint.go

+			// If the context was not specified, we set the index to be the max index
+			// from  available contexts
+			reply.Index = 0
+			for k, v := range reply.Matches {


I don't think you want to iterate over the matches. I think you want to do it over the contexts.

Should the max index be used even if the specified prefix does not have any matches for that particular context?

dadgar · 2017-08-04T23:22:54Z

nomad/structs/structs.go

+	// Prefix is what resources are matched to. I.e, if the given prefix were
+	// "a", potential matches might be "abcd" or "aabb"
+	Prefix string
+	// Context is the resource that can be matched. A context can be a job, node,


White space above

dadgar · 2017-08-04T23:23:34Z

nomad/resources_endpoint_test.go

+	assert.Equal(uint64(1000), resp.Index)
+}
+
+//// Tests that the top 20 matches are returned when no prefix is set


Double comments?

dadgar · 2017-08-07T16:40:40Z

nomad/resources_endpoint.go

@@ -47,8 +47,7 @@ func getMatches(iter memdb.ResultIterator) ([]string, bool) {
 		case *structs.Node:
 			id = raw.(*structs.Node).ID
 		default:
-			//s.logger.Printf("[ERR] nomad: unexpected type for resources context; not a job, allocation, node, or evaluation")
-			// TODO
+			r.srv.logger.Printf("[ERR] nomad.resources: unexpected type for resources context; %T \n", t)


Can you change ; to : just for consistency and you also don't need the \n. The logger adds it automatically.

dadgar · 2017-08-07T16:45:07Z

nomad/resources_endpoint.go

+		isTruncated = true
+	}
+
+	return matches, isTruncated


An option that you don't have to take:

return matches, iter.Next() != nil

That looks great, adding it.

dadgar · 2017-08-07T16:45:49Z

nomad/resources_endpoint.go

+			// 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.
+			reply.Index = 0


It is already initialized at zero

github-actions · 2023-03-27T02:11:10Z

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.

chelseakomlo added 20 commits August 4, 2017 14:34

Retrieve job information for resources endpoint

1b24ae5

requires further refactoring and logic for more contexts

adding test validation that received resources matches requested

cb1e898

test resources endpoint will return matching prefixes

c3e0bd8

remove unnecessary validations; these are tested elsewhere

0bd9742

refactor test helper

5435773

limit resources results to 20

4945b98

remove resourceliststub, no need for another layer of abstraction

1ec5010

change resources endpoint from http get to post

9e06777

adds evaluations

dfd8c1d

makes context singular

add truncation boolean to response

e14b279

use upsert as a test helper

a9ed750

refactor to remove duplication for types of resources

ed8e707

adding allocations to resouces list endpoint

a9728c3

adding nodes to resources list endpoint

refactor and add error handling for invalid context type

a3234b2

resources list endpoint accepts http POST and PUT

760f429

set the index for a resources response

refactor rpc endpoint and tests

c9c7b19

add test for when no prefixes are matched add test for no context at HTTP api

add documentation

9947939

extract magic number into variable

set response index and meta information

01640d8

add tests for edge cases

resources are expected by state store to be plural

ca4ed64

further refactoring

96a1723

dadgar requested changes Aug 4, 2017

View reviewed changes

chelseakomlo added 3 commits August 4, 2017 19:48

fix up tests to intantiate assertion test helper

971183d

add http tests for remaining contexts

if no context is specified, set maximum index for available contexts

74f35dd

further refactoring

b2df34c

dadgar requested changes Aug 4, 2017

View reviewed changes

chelseakomlo added 2 commits August 7, 2017 14:26

syntax fixups and logging

753b157

max index for any resource, if context is unspecified

e5e4fc4

dadgar approved these changes Aug 7, 2017

View reviewed changes

chelseakomlo added 2 commits August 7, 2017 17:28

code simplifications and logging

a1b5925

update changelog

440fea7

chelseakomlo force-pushed the f-autocomplete-api branch from daef46f to 440fea7 Compare August 7, 2017 19:26

chelseakomlo merged commit a768eca into master Aug 7, 2017

chelseakomlo deleted the f-autocomplete-api branch August 7, 2017 19:40

chelseakomlo mentioned this pull request Aug 9, 2017

Allocation autocomplete, client api #2995

Merged

github-actions bot locked as resolved and limited conversation to collaborators Mar 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

autocomplete api #2964

autocomplete api #2964

chelseakomlo commented Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

chelseakomlo Aug 7, 2017

dadgar Aug 4, 2017

dadgar Aug 4, 2017

dadgar Aug 7, 2017

dadgar Aug 7, 2017

chelseakomlo Aug 7, 2017

dadgar Aug 7, 2017

github-actions bot commented Mar 27, 2023


		res := resp.(structs.ResourcesResponse)

		assert.Equal(t, 1, len(res.Matches))

autocomplete api #2964

autocomplete api #2964

Conversation

chelseakomlo commented Aug 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Mar 27, 2023