-
Notifications
You must be signed in to change notification settings - Fork 11
Add grpc helpers to go #35
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
Conversation
Required by src-d/lookout-gometalint-analyzer#17 |
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.
LGTM, with minor comments
language-analyzer.go
Outdated
|
||
type analyzer struct{} | ||
|
||
var portToListen = 2020 | ||
var dataSrvAddr = "localhost:10301" | ||
var dataSrvAddr, _ = pb.ToGoGrpcAddress("ipv4://localhost:10301") | ||
var version = "alpha" | ||
var maxMessageSize = 100 * 1024 * 1024 //100mb |
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.
nitpicking, but let's change this comment to 100MB
pb/grpc.go
Outdated
) | ||
|
||
// maxMessageSize overrides default grpc max. message size to send/receive to/from clients | ||
var maxMessageSize = 100 * 1024 * 1024 // 100mb |
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.
same, let's change this comment to 100MB
let's keep it open until there is a PR with python helpers to make sure they aren't too different. |
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.
lgtm
I'd merge and create an issue to implement it in a next iteration |
I'm currently working on python implementation. But I'm afraid it can be much different from Go one. It's better to review both of them before merging. To make sure they are consistent and decide which inconsistencies are acceptable (languages are different so the code can't be absolutely the same) |
Signed-off-by: Maxim Sukharev <max@smacker.ru>
Signed-off-by: Maxim Sukharev <max@smacker.ru>
Signed-off-by: Maxim Sukharev <max@smacker.ru>
Signed-off-by: Maxim Sukharev <max@smacker.ru>
Analyzer server doesn't need increased grpc message size It sends comments back, not UAST. Signed-off-by: Maxim Sukharev <max@smacker.ru>
Analyzers don't need it and there is no equivalent in python-sdk Signed-off-by: Maxim Sukharev <max@smacker.ru>
|
Move low-level gRPC helpers from lookout to sdk.
Currently, URI schema is a huge mess. Some analyzers support RFC 3986 others not. Infra suffered from it already. This PR brings helpers to sdk and mention them as best-practice.
We allow to change
maxMessageSize
in lookoutd but it doesn't make much sense to have increased size only on server or client side. Better if they always match.Similar helpers are implemented in python by #37.