-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
NATS transport for go-kit #623
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is currently WIP, but I still did a quick glimpse over the code and added some comments. Probably missed some obvious things.
kitprometheus "github.com/go-kit/kit/metrics/prometheus" | ||
httptransport "github.com/go-kit/kit/transport/http" | ||
natstransport "github.com/go-kit/kit/transport/nats" | ||
"fmt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stdlib packages should all be in the first block, see https://github.com/golang/go/wiki/CodeReviewComments#imports
|
||
nc, err := nats.Connect(urls) | ||
|
||
defer nc.Close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
err should be checked before deferring close, otherwise this could fail
|
||
import ( | ||
"context" | ||
"github.com/nats-io/go-nats" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stdlib imports should be in there own block, see https://github.com/golang/go/wiki/CodeReviewComments#imports
|
||
"github.com/go-kit/kit/endpoint" | ||
"github.com/nats-io/go-nats" | ||
log "github.com/sirupsen/logrus" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use external logger packages. go-kit has it's own logger package that should be used instead.
// MsgHandler implements the MsgHandler type. | ||
func (s Server) MsgHandler(msg *nats.Msg) { | ||
|
||
urls := fmt.Sprint(os.Getenv("NATS_SERVER"), ", ", fmt.Sprintf("nats://%v:%v", os.Getenv("NATS_SERVICE_HOST"), os.Getenv("NATS_SERVICE_PORT"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should get the original NATS connection instead of reading some environment variables and trying to connect to a NATS server.
Unfortunately there is currently no way to get the connection from the message. I think it would be best to just add a method in the go-nats package that allows replying directly on a *nats.Msg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened nats-io/go-nats#323 for this.
|
||
// ServerAfter functions are executed on the HTTP response writer after the | ||
// endpoint is invoked, but before anything is written to the client. | ||
func ServerAfter(after ...ServerResponseFunc) ServerOption { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The appended functions are not used anywhere
import ( | ||
"context" | ||
|
||
"fmt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stdlib imports should be a single block, not two
// metadata headers to be transported to the server. ClientRequestFuncs are | ||
// executed after creating the request but prior to sending the gRPC request to | ||
// the server. | ||
type ClientRequestFunc func(context.Context, *metadata.MD) context.Context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this take a GRPC metadata map? NATS also has no way to send/receive metadata, so the you could do is pass the raw *nats.Msg.
This whole file currently makes not much sense, since it is written for a GRPC transport.
return | ||
} | ||
|
||
payload, err := s.enc(ctx, response) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the message has no reply topic set? In this case there is no need for for encoding the response, since no one can receive it. This should probably just return before encoding the response, if there is no reply topic.
defer nc.Close() | ||
|
||
// Non-nil non empty context to take the place of the first context in th chain of handling. | ||
ctx := context.TODO() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since NATS doesn't provide us with a context or similiar this could probably be just context.Background().
Any updates on this one? Without some positive comment in the next couple days, I'll likely close. |
This is fixed now by #680 |
A implementation utilizing the NATS protocol
Description
NATS is a messaging protocol written in go similar to RabbitMQ, an encoder, decoder, and server has been adapted from the existing http and grpc examples.
Motivation and Context
We desired to use a light weight messaging protocol for microservice communication that was very light on the ceremony. We were very interested in NATS an adopted it to operate within the go-kit framework.
How Has This Been Tested?
Tests are coming
Screenshots (if appropriate):
Types of changes
Checklist: