add metaclient #130
add metaclient #130
Conversation
There was a problem hiding this comment.
Reviewed 5 of 6 files at r1.
Reviewable status: 5 of 6 files reviewed, 2 unresolved discussions (waiting on @lwsanty)
metaclient.go, line 45 at r1 (raw file):
// NewMetaClient is a MetaClient constructor, used to parse and save MetaClientConfig from a given filepath func NewMetaClient(configPath string) (*MetaClient, error) {
Assuming the comment about configs vs DSN addresses, we can hide this implementation into the existing client without changing the user-facing API.
_testdata/endpoints.yml, line 1 at r1 (raw file):
python: localhost:9432
I'd rather pass those configs as an address to the client, similar to how DSN works for sql package.
In local case you can pass something like: python=localhost:9432,go=localhost:9432.
For K8S I think it will be easier to have a template for an address.
For example, assuming each service in K8S gets this name:
my-svc.my-namespace.svc.example.com
And in case we have those names for driver services:
go-driver.bblfsh.svc.example.com
python-driver.bblfsh.svc.example.com
So we can pass this address to the client and let it fill the template with a language parameter: %s-driver.bblfsh.svc.example.com
The parser for DSN can be naive and assume that:
- If
,is present, parse aslanguage=addr,...pairs. - If
%is present, parse as a single template address. - Interpret as a single address otherwise (single language or bblfshd setup).
There was a problem hiding this comment.
Reviewable status: 1 of 6 files reviewed, 2 unresolved discussions (waiting on @dennwc)
metaclient.go, line 45 at r1 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
Assuming the comment about configs vs DSN addresses, we can hide this implementation into the existing client without changing the user-facing API.
Done.
_testdata/endpoints.yml, line 1 at r1 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
I'd rather pass those configs as an address to the client, similar to how DSN works for
sqlpackage.In local case you can pass something like:
python=localhost:9432,go=localhost:9432.For K8S I think it will be easier to have a template for an address.
For example, assuming each service in K8S gets this name:
my-svc.my-namespace.svc.example.comAnd in case we have those names for driver services:
go-driver.bblfsh.svc.example.com python-driver.bblfsh.svc.example.comSo we can pass this address to the client and let it fill the template with a language parameter:
%s-driver.bblfsh.svc.example.comThe parser for DSN can be naive and assume that:
- If
,is present, parse aslanguage=addr,...pairs.- If
%is present, parse as a single template address.- Interpret as a single address otherwise (single language or bblfshd setup).
Done.
There was a problem hiding this comment.
Reviewed 8 of 8 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @lwsanty)
client.go, line 36 at r2 (raw file):
var ( // ErrLangNotSupported is the error that must be returned if requested language driver is not supported/available ErrLangNotSupported = errors.NewKind("language %q is not supported in current configuration")
I think this kind of error is already defined by SDK.
client.go, line 38 at r2 (raw file):
ErrLangNotSupported = errors.NewKind("language %q is not supported in current configuration") // ErrNotImplemented is the error that must be returned if some methods are not implemented ErrNotImplemented = errors.NewKind("not implemented")
Since it's only used by gRPC, you should use a NOT_IMPLEMENTED gRPC error instead.
client.go, line 235 at r2 (raw file):
func (c *Client) Close() error { return c.conn.Close()
OK, not the conn should probably become a io.Closer and in this case it will store the connection, for others it will store a multi client.
driver-client.go, line 16 at r2 (raw file):
parses on the large scale of go- and python-driver containers/instances etc. Solution:
It should probably be in the Babelfish docs. Here we can keep the endpoint examples.
driver-client.go, line 31 at r2 (raw file):
// MultipleDriverClient is a DriverClient implementation, contains connection getter and a map[language]connection type MultipleDriverClient struct {
Please unexport it. It's an implementation detail.
driver-client.go, line 31 at r2 (raw file):
// MultipleDriverClient is a DriverClient implementation, contains connection getter and a map[language]connection type MultipleDriverClient struct {
It's missing a Close method to cleanup the map.
driver-client.go, line 33 at r2 (raw file):
type MultipleDriverClient struct { getConn getConnFunc Languages map[string]*grpc.ClientConn
Unexport
driver-client.go, line 60 at r2 (raw file):
return nil, err } conn = gConn
Forgot to add to the map?
driver-client.go, line 63 at r2 (raw file):
} return protocol2.NewDriverClient(conn).Parse(ctx, in, opts...)
It's better to keep the client instead of creating it each time. And you will need to keep both the client and the connection.
go.mod, line 14 at r2 (raw file):
) replace github.com/bblfsh/sdk/v3 v3.2.5 => github.com/lwsanty/sdk/v3 v3.2.1-0.20191112180841-f3c9d3e2f493
Blocking until we merge that PR.
There was a problem hiding this comment.
Reviewable status: 3 of 7 files reviewed, 10 unresolved discussions (waiting on @dennwc and @lwsanty)
client.go, line 36 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
I think this kind of error is already defined by SDK.
Done.
client.go, line 38 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
Since it's only used by gRPC, you should use a
NOT_IMPLEMENTEDgRPC error instead.
Done.
client.go, line 235 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
OK, not the
connshould probably become aio.Closerand in this case it will store the connection, for others it will store a multi client.
please expand your request, I'm afraid I cannot get it.
driver-client.go, line 16 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
It should probably be in the Babelfish docs. Here we can keep the endpoint examples.
Done.
driver-client.go, line 31 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
Please unexport it. It's an implementation detail.
Done.
driver-client.go, line 31 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
It's missing a
Closemethod to cleanup the map.
Done.
driver-client.go, line 33 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
Unexport
Done.
driver-client.go, line 60 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
Forgot to add to the map?
Done.
driver-client.go, line 63 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
It's better to keep the client instead of creating it each time. And you will need to keep both the client and the connection.
Done.
There was a problem hiding this comment.
Reviewed 3 of 4 files at r3.
Reviewable status: 6 of 7 files reviewed, 7 unresolved discussions (waiting on @dennwc and @lwsanty)
client.go, line 235 at r2 (raw file):
Previously, lwsanty (Oleksandr Chabaiev) wrote…
please expand your request, I'm afraid I cannot get it.
The type of c.conn should be io.Closer. Also, this field should be populated with MultiClient to allow it to close all connections when the client is closed.
client.go, line 67 at r3 (raw file):
e, ok := endpoints[lang] if !ok { return nil, server.ErrUnsupportedLanguage.New(lang)
Ouch. Is it only exposed by this package? We shouldn't import a server package in the client.
driver-client.go, line 63 at r2 (raw file):
Previously, lwsanty (Oleksandr Chabaiev) wrote…
Done.
I still see that you don't add it to the map and create a new one each time.
driver-client.go, line 30 at r3 (raw file):
type ( languageConnection map[string]*grpc.ClientConn
How about defining a type with both the connection and the client and having a single map?
driver-client.go, line 72 at r3 (raw file):
func (c *multipleDriverClient) Close() error { c.langConns = make(languageConnection)
You need to actually close each connection here.
request.go, line 178 at r3 (raw file):
} // SupportedLanguagesRequest is a request to retrieve the supported langConns.
Incorrect refactor?
lwsanty
force-pushed
the
meta-client
branch
2 times, most recently
from
9b81118
to
3967cd8
Compare
lwsanty
force-pushed
the
meta-client
branch
2 times, most recently
from
6cb1c51
to
cd8cbe0
Compare
…s on the large scale Scenario: we need to parse a ton of go and python files inside the k8s environment, to save time we need to perform this parses on the large scale of go- and python-driver containers/instances etc. Solution: - run two separate Deployments of go- and python-driver container pods - provide Horizontal Autoscalers for both of Deployments - provide Services with LoadBalancer type - during client initialization provide Services endpoints configuration, this will create two language-oriented connections that will be responsible for sending parse request only to a dedicated Service, that will load-balance this requestbetween underlying language driver pods Examples of endpoint formats: - localhost:9432 - casual example there's only one driver or bblfshd server - python=localhost:9432,go=localhost:9432 - coma-separated mapping in format language=address - %s-driver.bblfsh.svc.example.com - DNS template based on the language Signed-off-by: lwsanty <[email protected]>
There was a problem hiding this comment.
Reviewed 3 of 5 files at r4, 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @lwsanty)
Signed-off-by: lwsanty <[email protected]>
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @dennwc)
client.go, line 67 at r3 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
Ouch. Is it only exposed by this package? We shouldn't import a server package in the client.
Done.
driver-client.go, line 30 at r3 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
How about defining a type with both the connection and the client and having a single map?
Done.
driver-client.go, line 72 at r3 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
You need to actually close each connection here.
Done.
go.mod, line 14 at r2 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
Blocking until we merge that PR.
Done.
request.go, line 178 at r3 (raw file):
Previously, dennwc (Denys Smirnov) wrote…
Incorrect refactor?
Done.
Signed-off-by: lwsanty <[email protected]>
Signed-off-by: lwsanty <[email protected]>
There was a problem hiding this comment.
Reviewed 2 of 2 files at r6, 2 of 2 files at r7, 1 of 1 files at r8.
Reviewable status:complete! all files reviewed, all discussions resolved
MultipleDriverClient could be useful during language-specific parsings on the large scale
Scenario: we need to parse a ton of go and python files inside the k8s environment, to save time we need to perform this parses on the large scale of go- and python-driver containers/instances etc.
Solution:
Examples of endpoint formats:
localhost:9432- casual example there's only one driver or bblfshd serverpython=localhost:9432,go=localhost:9432- coma-separated mapping in formatlanguage=address%s-driver.bblfsh.svc.example.com- DNS template based on the languageSigned-off-by: lwsanty [email protected]
This change is