-
Notifications
You must be signed in to change notification settings - Fork 23
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
Zipper CarbonV2 gRPC render stream #370
Conversation
4e19da3
to
e4a6a8c
Compare
e4a6a8c
to
0b22237
Compare
protocolBackends: | ||
- http: "http://10.291.202.31:8080" | ||
- http: "http://10.291.197.91:8080" | ||
grpc: "10.291.197.91:7070" |
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.
it's minor, but you can simplify things by:
protocolBackends:
- "http://10.291.202.31:8080"
- "http://10.291.197.91:8080"
"grpc://10.291.197.91:7070"
net/url pkg actually supports parsing it: https://pkg.go.dev/net/url@go1.18.4#Parse
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 have difficulty understanding the structure of protocolBackend
. Do you mean it can be simplified to list of strings? In that case it would be like:
protocolBackends:
- ["http://10.291.202.31:8080"]
- ["http://10.291.197.91:8080",
"grpc://10.291.197.91:7070"]
Which is not desired IMO. However, after implementing gRPC for all of the calls, it can be simplified to one string in which url scheme determines the backend type. E.g.
protocolBackends:
- "http://10.291.202.31:8080"
- "grpc://10.291.197.91:7070"
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.
oops, sorry, the example that I gave wasn't correct, I wanted to suggest an array of array:
protocolBackends:
- "http://10.291.202.31:8080"
- - "http://10.291.197.91:8080"
- "grpc://10.291.197.91:7070"
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.
left one more small comment. but not a blocker and the rest seems good to me, so approved.
(I will leave the real check to production. :P)
This PR adds the functionality of gRPC streaming Renders in carbonzipper. The new gRPC protocol is proposed at go-graphite/protocol#12, and storage part is implemented on go-graphite/go-carbon#476.
Outline:
A new backend configuration structure is proposed as
ProtocolBackend
which defines different protocol addresses of a backend. The change is backward compatible.The current implementation only uses gRPC for Renders as we have not implemented other APIs here or in go-carbon. For the other APIs such as Find, old methods are used.
The changes are tested manually in container environment.
Some works should be done before merging: