-
Notifications
You must be signed in to change notification settings - Fork 59
Refactor CommonClient implementation and add SSL support for gRPC #37
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
base: develop
Are you sure you want to change the base?
Refactor CommonClient implementation and add SSL support for gRPC #37
Conversation
|
@Jeongseup would appreciate your thoughts on this change 🙏 |
|
Thank you It is wonderful. I thought also CVMS has to have a test mock client and db for new package implementation. But As you already know, This PR has HUGE code replacement. so.. I cannot check all about this. Please wait for it. I want to make a several new packages for Babylon and Axelar Amplifier. |
|
Sure no worries! After all this is quite a change. We run my forked CVMS and so far it works quite well. Only thing is it increased memory consumption a bit. Sometimes memory consumption spikes for a very short time before it's back to normal. I did not find the reason yet, but just something to be aware if you defined any memory usage quotas. Oh nice I also wanted to take a look at getting axelar amplifier support into CVMS, but had no time yet :( |
|
Ah I remembered it! When I developed this project a few months ago, There was memory leak in making a client (not requester). I think we should check memory leak before merging this PR. For that, I already implemented debug handle at https://github.com/cosmostation/cvms/blob/develop/internal/app/exporter/exporter.go#L56 |
Signed-off-by: Simon Lichtenauer <[email protected]>
* started working on common client interface abstracting different concrete clients Signed-off-by: Simon Lichtenauer <[email protected]> * migrated APIClient to use abstract client implementation instead of resty directly Signed-off-by: Simon Lichtenauer <[email protected]> * refactored grpc client use to use common abstract client; Fixed common client to use global application logger Signed-off-by: Simon Lichtenauer <[email protected]> * Added changes to GRPC client to support SSL and insecure connections Signed-off-by: Simon Lichtenauer <[email protected]> * moved client implementation into own package to ensure its only used indirectly Signed-off-by: Simon Lichtenauer <[email protected]> * moved grpchelper methods into the grpc client implementation. Removed no longer used grpc helper methods Signed-off-by: Simon Lichtenauer <[email protected]> * add mock and demo it with simple happypath test for uptime module Signed-off-by: Simon Lichtenauer <[email protected]> * add changes to be compatible with latest changes in cosmostation/develop branch Signed-off-by: Simon Lichtenauer <[email protected]> --------- Signed-off-by: Simon Lichtenauer <[email protected]>
Signed-off-by: Simon Lichtenauer <[email protected]>
…LS check timesout (#11) Signed-off-by: Simon Lichtenauer <[email protected]>
…ges from new client implementation Signed-off-by: Simon Lichtenauer <[email protected]>
28a6494 to
370743a
Compare
|
Makes sense. The memory usage pattern looked like a memory leak to me. I'll see if I can find something. I just rebased to keep this draft compatible with your upstream branch |
|
Could you share test GRPC endpoint for TLS? I used my one but not working. I want to check your endpoint format. |
|
@Jeongseup It should be as simple as using: grpc: 'band-grpc.polkachu.com:22990' or grpc: 'axelar-grpc.publicnode.com:443' Also sorry for me not keeping this PR up to date. Was busy with other thing, but I plan to rebase as soon as I find some time |
|
Nob! I can understand your busy nowadays. Actullay I wanted to add golangci-lint for our project. for that, I got too many errors... caused by deprecated grpc client. and also your PR is very good implementation. and I'm making a branch for this. After creating new branch, Please check it. that branch is about up to date for this PR |
|
Actually I don't want to cosmos-sdk dependency if it is possible, that's why I used dynamic jsonpb, but this lib was deprecated now. So, I think we should remove cosmos-sdk.io depencies to make this PR. Expect that, I think that your PR for creating public client interface was REALLY useful and beautiful codes. Thanks! |
|
@Jeongseup yeah I understand the issue with depending on cosmos-sdk, However there's no real simple solution to actually deserialize the protobuf objects into usable go objects. The dependency I added is actually just the |
PR(Pull Request) Overview
Rework of the
CommonClientimplementation and adding support of SSL encrypted gRPC endpointsChanges
Description of Changes
This is a major change of the
CommonClienttype by instead of usingrestydirectly the client implementations are hidden behind an abstractclient.Clientinterface. This allows to implement various clients for different use-cases (e.g. grpc) and also simplifies testing massively by allowing to useclient.Mockto test the API related methods. An example of this is the added unit test for thegetValidatorUptimeStatusmethod fromuptime/api.go(https://github.com/cosmostation/cvms/compare/develop...qwertzlbert:cvms:refacture-client-implementation?expand=1#diff-8cc62f3c8463e0096e5b2689cd1dbad8d56d2b79af139f1072eced745bd5ee24) .Additionally I reworked the gRPC implementation to now also support SSL connections and rely on the newer google gRPC packages instead of the deprecated packages used previously.
Using a gRPC connection is now much simpler as the client also implements the
client.Clientinterface as you can see from the changes added to theeventnoncepackage.Testing Method
client.MockAdditional Information
Consider this PR as a work in progress for now, while it seems to work testing is ongoing. I just wanted to open the PR now already to get some feedback to this change