allow lookup by prefix on ID and full Name for jobs and nodes by vieux · Pull Request #210 · moby/swarmkit

vieux · 2016-03-25T00:16:53Z

result from bench:

BenchmarkCreateNode-4                 100000         23368 ns/op        4800 B/op         70 allocs/op
BenchmarkUpdateNode-4                  50000         27459 ns/op        6536 B/op         86 allocs/op
BenchmarkUpdateNodeTransaction-4       50000         34427 ns/op       10004 B/op        152 allocs/op
BenchmarkDeleteNodeTransaction-4      100000         13753 ns/op        4368 B/op         66 allocs/op
BenchmarkGetNode-4                   1000000          1770 ns/op         272 B/op          9 allocs/op
BenchmarkFindAllNodes-4                  200       7237776 ns/op     1666273 B/op      30030 allocs/op
BenchmarkFindNodeByName-4            1000000          1366 ns/op         185 B/op         11 allocs/op
BenchmarkNodeConcurrency-4             10000        156862 ns/op       43126 B/op        737 allocs/op

Compared to #199:

benchmark                            old ns/op     new ns/op     delta
BenchmarkCreateNode-4                49162         23368         -52.47%
BenchmarkUpdateNode-4                64126         27459         -57.18%
BenchmarkUpdateNodeTransaction-4     74439         34427         -53.75%
BenchmarkDeleteNodeTransaction-4     35235         13753         -60.97%
BenchmarkGetNode-4                   1900          1770          -6.84%
BenchmarkFindAllNodes-4              11709034      7237776       -38.19%
BenchmarkFindNodeByName-4            1415          1366          -3.46%
BenchmarkNodeConcurrency-4           328975        156862        -52.32%

benchmark                            old allocs     new allocs     delta
BenchmarkCreateNode-4                154            70             -54.55%
BenchmarkUpdateNode-4                208            86             -58.65%
BenchmarkUpdateNodeTransaction-4     310            152            -50.97%
BenchmarkDeleteNodeTransaction-4     155            66             -57.42%
BenchmarkGetNode-4                   9              9              +0.00%
BenchmarkFindAllNodes-4              30588          30030          -1.82%
BenchmarkFindNodeByName-4            11             11             +0.00%
BenchmarkNodeConcurrency-4           1450           737            -49.17%

benchmark                            old bytes     new bytes     delta
BenchmarkCreateNode-4                11590         4800          -58.58%
BenchmarkUpdateNode-4                15051         6536          -56.57%
BenchmarkUpdateNodeTransaction-4     21684         10004         -53.86%
BenchmarkDeleteNodeTransaction-4     10911         4368          -59.97%
BenchmarkGetNode-4                   272           272           +0.00%
BenchmarkFindAllNodes-4              2340975       1666273       -28.82%
BenchmarkFindNodeByName-4            185           185           +0.00%
BenchmarkNodeConcurrency-4           96820         43126         -55.46%

use .Find(ByPrefix(xxx)) use prefix from go-memdb Signed-off-by: Victor Vieux <vieux@docker.com>

docker-codecov-bot · 2016-03-25T00:23:11Z

Current coverage is `56.50%`

Merging #210 into master will decrease coverage by -0.74% as of 4eda9d8

@@            master   #210   diff @@
=====================================
  Files           43     43       
  Stmts         5220   5281    +61
  Branches       745    755    +10
  Methods          0      0       
=====================================
- Hit           2988   2984     -4
- Partial        406    411     +5
- Missed        1826   1886    +60

Review entire Coverage Diff as of 4eda9d8

Uncovered Suggestions

+0.43% via agent/session.go#185...207
+0.34% via agent/agent.go#524...541
+0.34% via .../test/deepcopy.pb.go#1492...1509
See 7 more...

Powered by Codecov. Updated on successful CI builds.

aluzzardi · 2016-03-25T00:59:44Z

Open question: Should it be ID prefix, Name prefix or both? Right now we're doing both and I'm not sure about names.

Also, the only way to do a name lookup through the API is by prefix.

The following would be possible, but I'm not sure I like it:

c.ListJobs(&api.ListJobsRequest{
  options: &api.ListOptions{
    name: args[0],
    prefix: args[0],
  },
})

vieux · 2016-03-25T01:18:14Z

@aluzzardi I think it should be only ID I added names here to have an apple to apple comparaison with #199

aaronlehmann · 2016-03-25T01:23:23Z

I would also vote to only do prefix matching on IDs, but I don't have a strong opinion. The argument in favor of doing it is that it's more consistent if we do prefix matches on both names or IDs, so there isn't an arbitrary difference between them that people have to be aware of. The argument against is that typing a name shouldn't be a big burden to place on users, and by keeping names explicit we can avoid confusing "ambiguous name..." error messages.

vieux · 2016-03-25T01:24:36Z

@aaronlehmann I agree. Are you OK to close #199 ? if so I'll update this on to prefix only IDs

aluzzardi · 2016-03-25T01:28:41Z

@vieux @aaronlehmann

The question is, how do we lookup by name then? :)

There are two options:

Either Get*() allows for Names, or
List*() has some way to specify a name (in the same way you'd specify a prefix or labels)

If we go with 1) then swarmctl job inspect foo would have to attempt a GetJob(args[0]) then a ListJobs(prefix=args[0]).
If we go with 2), then it would be ListJobs(name=args[0], prefix=args[0]) (may be proceeded by Get(args[0]) if we want to attempt an exact ID lookup first without collision risk)

vieux · 2016-03-25T01:31:23Z

@aluzzardi I would go with 2) but I don't like the fact that you pass the same arg twice.

Why not just une generic parameter like ListJobs(term=args[0])

aaronlehmann · 2016-03-25T01:32:01Z

@aluzzardi: Either approach is fine with me.

@vieux: Feel free to close #199.

aaronlehmann · 2016-03-25T01:33:51Z

manager/state/memory.go

 		}
 		return fromResultIterator(it), nil
+	case byPrefix:
+		itID, err := jobs.memDBTx.Get(jobs.table(), indexID+"_prefix", string(v))


Can you put "_prefix" in a constant? I know it's a little silly, but I've been burned too many times by typos in strings like this.

aaronlehmann · 2016-03-25T01:35:34Z

The store parts look good to me. Just need to finalize the details currently being discussed, then we should merge this.

vieux · 2016-03-25T01:45:21Z

@aluzzardi or we do something very generic,we use index directly : ListJobs(indexId_prefix=args[0], indexName=args[0])

vieux · 2016-03-25T19:55:43Z

@aaronlehmann I addressed all your comments

@aluzzardi I went with option 2, with an match via full ID first.
I think it's good to merge as is, and we might revisit the args to ListXXX later

aaronlehmann · 2016-03-25T20:32:31Z

api/cluster.proto


+message ListOptions {
+	string prefix = 1;
+}


Seems a little strange that this prefix field can also match a full name. But I'm not really sure what else to suggest.

Maybe "query"?

aaronlehmann · 2016-03-25T21:04:56Z

LGTM

aluzzardi · 2016-03-25T21:15:58Z

@aaronlehmann @vieux Not sure about query. Eventually, ListOptions will have labels, namespace etc.

vieux · 2016-03-25T21:16:30Z

@aluzzardi agreed but for now it's good enough no ?

aluzzardi · 2016-03-25T21:44:26Z

I have some reservations on name prefix lookup. I'm not exactly sure, but I would have probably just used ByIDPrefix in the store and let the user specify a name in the options if he wanted to lookup by name.

It's more flexible and it's going to get more complex when we add namespaces (Find by ID prefix or namespace+name prefix). Also, I don't know if we want to implicitly do a name prefix lookup.

Even though it's more calls, what about, when you run swarmctl job inspect foo, the CLI does:

GetJob(args[0])
ListJobs(
  Namespace: flags.Namespace,
  Name: args[0],
)
ListJobs(
  Prefix: args[0],
)

Note that in gRPC, there's just one connection so issuing a bunch of calls is not that expensive.

I'm okay with merging it as is and improving later on.

LGTM

vieux · 2016-03-25T21:46:16Z

@aluzzardi note: in this PR we only match on full Name and partial ID. there is no prefix search on name.

Feel free to merge

aluzzardi · 2016-03-26T03:15:07Z

@vieux Woops - forgot to say, we should validate that the query has at least 2 characters.

stevvooe · 2016-03-29T21:26:06Z

api/cluster.proto

 }

+message ListOptions {
+	string query = 1;


Can we have a TODO here? This should not be a permanent type. It doesn't take on any of the design constraints we've spoken about.

remove name hack for jobs

4eda9d8

use .Find(ByPrefix(xxx)) use prefix from go-memdb Signed-off-by: Victor Vieux <vieux@docker.com>

GordonTheTurtle added the status/0-triage label Mar 25, 2016

vieux mentioned this pull request Mar 25, 2016

[WIP] allow lookup by prefix on ID and Name for jobs #199

Closed

aaronlehmann reviewed Mar 25, 2016
View reviewed changes

vieux changed the title ~~allow lookup by prefix on ID and Name for jobs and nodes~~ allow lookup by prefix on ID and full Name for jobs and nodes Mar 25, 2016

aluzzardi merged commit 2f303c7 into moby:master Mar 26, 2016

aluzzardi deleted the prefix_no_index branch March 26, 2016 00:47

stevvooe reviewed Mar 29, 2016
View reviewed changes

Conversation

vieux commented Mar 25, 2016

Uh oh!

docker-codecov-bot commented Mar 25, 2016

Current coverage is 56.50%

Uncovered Suggestions

Uh oh!

aluzzardi commented Mar 25, 2016

Uh oh!

vieux commented Mar 25, 2016

Uh oh!

aaronlehmann commented Mar 25, 2016

Uh oh!

vieux commented Mar 25, 2016

Uh oh!

aluzzardi commented Mar 25, 2016

Uh oh!

vieux commented Mar 25, 2016

Uh oh!

aaronlehmann commented Mar 25, 2016

Uh oh!

aaronlehmann Mar 25, 2016

Choose a reason for hiding this comment

Uh oh!

vieux Mar 25, 2016

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 25, 2016

Uh oh!

vieux commented Mar 25, 2016

Uh oh!

vieux commented Mar 25, 2016

Uh oh!

aaronlehmann Mar 25, 2016

Choose a reason for hiding this comment

Uh oh!

vieux Mar 25, 2016

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Mar 25, 2016

Uh oh!

aluzzardi commented Mar 25, 2016

Uh oh!

vieux commented Mar 25, 2016

Uh oh!

aluzzardi commented Mar 25, 2016

Uh oh!

vieux commented Mar 25, 2016

Uh oh!

aluzzardi commented Mar 26, 2016

Uh oh!

stevvooe Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Current coverage is `56.50%`