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

apigateway example #203

Merged
merged 6 commits into from
Mar 18, 2016
Merged

apigateway example #203

merged 6 commits into from
Mar 18, 2016

Conversation

marshauf
Copy link
Contributor

@marshauf marshauf commented Mar 3, 2016

This is a work-in-progress pull request as discussed in #202 .

Running consul with the provided service definition, stringsvc3 and apigateway, you can run

for s in foo baar baz ; do curl -d"{\"s\":\"$s\"}" localhost:8000/api/stringsvc/uppercase ; done

instead of directly calling the stringsvc3 service method.

The apigateway currently only handles http json encoded calls. To support different transports I could check the consul service definition tags for values like "HTTP/JSON", "gRPC" to determine the transport and codec.

@marshauf
Copy link
Contributor Author

marshauf commented Mar 4, 2016

@peterbourgon Could you take a look at the last commit and tell me if that is what you have in mind, please.

@peterbourgon
Copy link
Member

Keeping my comments limited to the architecture for the time being...

  • Static declaration of proxied services is what I had in mind -- great
  • What is gained by the separate foosvc_def.json files, versus just hard-coding proxied services into the gateway?
  • Having to duplicate per-endpoint code in make{Concat,Sum}{Endpoint,Handler} feels like a red flag. This code shouldn't need to exist in the gateway. Could it be handled by per-service client packages (e.g. addsvc/client)? What are your thoughts?

@peterbourgon peterbourgon changed the title apigateway example [WIP] apigateway example Mar 7, 2016
@marshauf
Copy link
Contributor Author

marshauf commented Mar 7, 2016

The json files are consul definition files. The example services don't self-register with consul. I pass these files to consul on startup to let it know about the services which is I don't like but was the fastest way. I could add self-registration to the other example services or add a script which starts the service and does 3rd party registration.

Yes, I need to clean up the code. The make{Concat,Sum}Handler can be merged together. I lean towards having the endpoint and handler factory functions in the client/service packages, to keep the code each service adds, to the apigateway, to a minimum. It would make swapping service in and out of the apigateway simpler and reducing the imports to just one package per services instead for example addsvc, being two imports at the moment.

@peterbourgon
Copy link
Member

The json files are consul definition files.

Ahh, I see. For the purposes of this example, let's leave those out, and just assume that the operator is already running the services and has them registered in Consul. (We should add self-registration in #167.)

Yes, I need to clean up the code.

It's not just about cleaning up the existing code, it's about domain of responsibility. If the API gateway needs to unmarshal and remarshal requests, then it seems like that code should be owned by the respective services and called, rather than duplicated here. That in turn implies the services need either have client packages, or export the MakeFoo{Handler, Endpoint} methods — not sure which right now. Right? What do you think?

@marshauf
Copy link
Contributor Author

For the purposes of this example, let's leave those out, and just assume that the operator is already running the services and has them registered in Consul.

I agree, I removed the files.

What do you think?

Endpoint methods should be in a separate package, like netflix is handling it.
Reducing the amount of handlers to one per API. The handler takes the request body and passes it to the endpoint. The endpoint creates a request based on the data and calls the service endpoint. The result of the call is passed back to the handler and written to the client(ResponseWriter).

@peterbourgon
Copy link
Member

OK. Where do we stand here? Is the PR in a state that you consider final? Or are you looking for more direction from me?

@marshauf
Copy link
Contributor Author

I consider it final. It shows a basic apigateway, which could be used as a starting point. I like to keep the service endpoint factory code (eg. makeSumEndpoint) in the apigateway package because it is just relevant to the apigateway example. Should I squash the commits into one?

"github.com/hashicorp/consul/api"
"golang.org/x/net/context"
"google.golang.org/grpc"
)
Copy link
Member

Choose a reason for hiding this comment

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

Please separate these imports into 3 groups:

import (
    // stdlib

    // non go-kit

    // github.com/go-kit/kit/*
)

and remove the commented-out statements.

@peterbourgon
Copy link
Member

Here's a patch: https://gist.github.com/peterbourgon/4385034065703985e2ef

You can apply it with git apply /path/to/apigateway.patch

Changes:

  • Reorder imports
  • Remove flagset
  • Add loadbalancer.Retry and parameters
  • Restructure service definitions
    • Make ServiceDef an anonymous inline type
    • Rename the factory functions for symmetry
    • Add a lot of comments
    • Pass method as parameter to httpFactory
  • Passing "fatal" to logger.Log doesn't terminate; fix some instances of that
  • Consistent logger.Log "err" key with error vals
  • Changes to makeHandler
    • Use Retry-wrapped loadbalancer instead of invoking loadbalancer.Endpoint directly
    • Write http.Error to client in case of error

Does it make sense? Did I overlook anything? Do you like it? :)

If it's good, please commit and rebase on master, and we can get this merged!

@marshauf
Copy link
Contributor Author

The patch looks great.

Please put this down with the other goroutine pushing to errc. That would help reveal a bug: this channel needs a buffer size of 2 :)

I don't see a problem with it being an unbuffered channel. You also didn't change it in the patch.
As soon as an error value is received through errc the main function returns and exits the program.
Which should only happen on an interrupt signal or the http server returning an error. Am I missing something?

@peterbourgon
Copy link
Member

@marshauf You're right. In some circumstances it's important for all goroutines to be able to push into the channel even if there's no receivers, to prevent goroutine leaks. But in this case we just teardown immediately, so no problem.

Marcel Hauf added 6 commits March 18, 2016 12:51
…consul and forwards rpc calls to discovered endpoints
… variable to construct the apigateway mapping
* Reorder imports
* Remove flagset
* Add loadbalancer.Retry and parameters
* Restructure service definitions
	* Make ServiceDef an anonymous inline type
	* Rename the factory functions for symmetry
	* Add a lot of comments
	* Pass method as parameter to httpFactory
* Passing "fatal" to logger.Log doesn't terminate; fix some instances of that
* Consistent logger.Log "err" key with error vals
* Changes to makeHandler
	* Use Retry-wrapped loadbalancer instead of invoking loadbalancer.Endpoint directly
	* Write http.Error to client in case of error
@peterbourgon
Copy link
Member

Nice! Thanks for the contribution!

peterbourgon added a commit that referenced this pull request Mar 18, 2016
@peterbourgon peterbourgon merged commit ff58f40 into go-kit:master Mar 18, 2016
@peterbourgon peterbourgon changed the title [WIP] apigateway example apigateway example Mar 18, 2016
@peterbourgon peterbourgon mentioned this pull request Mar 25, 2016
guycook pushed a commit to codelingo/kit that referenced this pull request Oct 12, 2016
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