Skip to content

Conversation

smacker
Copy link
Contributor

@smacker smacker commented Nov 23, 2018

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.

@smacker
Copy link
Contributor Author

smacker commented Nov 23, 2018

Required by src-d/lookout-gometalint-analyzer#17

Copy link
Contributor

@carlosms carlosms left a 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


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
Copy link
Contributor

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
Copy link
Contributor

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

@smacker
Copy link
Contributor Author

smacker commented Nov 23, 2018

let's keep it open until there is a PR with python helpers to make sure they aren't too different.

Copy link
Contributor

@dpordomingo dpordomingo left a comment

Choose a reason for hiding this comment

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

lgtm

@dpordomingo
Copy link
Contributor

I'd merge and create an issue to implement it in a next iteration

@smacker
Copy link
Contributor Author

smacker commented Nov 26, 2018

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>
@smacker
Copy link
Contributor Author

smacker commented Nov 27, 2018

  1. rebased on master
  2. applied feedback
  3. don't use pb.NewServer in the example (don't need and to be consistent with python)

@smacker smacker merged commit d948dda into src-d:master Nov 28, 2018
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.

3 participants