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.

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

Adds support for prepared query templates. #1764

Merged
merged 18 commits into from
Mar 7, 2016
Merged

Conversation

slackpad
Copy link
Contributor

This is ready for review.

/cc @ryanuber and @sean-

@slackpad
Copy link
Contributor Author

slackpad commented Mar 3, 2016

Here are the benchmarks with the latest version of everything:

workpad:prepared_query james$ go test -bench .
PASS
BenchmarkTemplate_CompileSmall-4           10000            105945 ns/op
BenchmarkTemplate_CompileBig-4              5000            271096 ns/op
BenchmarkTemplate_RenderSmall-4            30000             39654 ns/op
BenchmarkTemplate_RenderBig-4              10000            107450 ns/op
ok      github.com/hashicorp/consul/consul/prepared_query       5.191s

The small render should be pretty typical of normal users, and 40 us seems pretty reasonable.


var ret structs.PreparedQueries
for wrapped := queries.Next(); wrapped != nil; wrapped = queries.Next() {
ret = append(ret, toPreparedQuery(wrapped))
Copy link
Member

Choose a reason for hiding this comment

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

Any reason to not just wrapped.(*queryWrapper).PreparedQuery? You already handle the nil check before we get here it looks like.

parse := func(path string, v reflect.Value) error {
tree, err := hil.Parse(v.String())
if err != nil {
return fmt.Errorf("Bad format '%s' in Service%s: %s", v.String(), path, err)
Copy link
Member

Choose a reason for hiding this comment

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

Does this render correctly without a space after Service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - this is a little cheesy but all paths have a leading "." so this looks like Service.Foo.

@ryanuber
Copy link
Member

ryanuber commented Mar 7, 2016

Code LGTM! 👍

slackpad added a commit that referenced this pull request Mar 7, 2016
Adds support for prepared query templates.
@slackpad slackpad merged commit 50e1326 into master Mar 7, 2016
@slackpad slackpad deleted the f-query-templates branch March 7, 2016 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants