Skip to content
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

EOS GRPC interface #1471

Merged
merged 51 commits into from
Jun 17, 2021
Merged

EOS GRPC interface #1471

merged 51 commits into from
Jun 17, 2021

Conversation

ffurano
Copy link
Contributor

@ffurano ffurano commented Feb 11, 2021

No description provided.

@ffurano ffurano requested a review from labkode as a code owner February 11, 2021 14:37
@ffurano
Copy link
Contributor Author

ffurano commented Feb 11, 2021

Why these things in the build fail, as they are not testing eosgrpc ? Maybe I have to wait some time and rebase ?

go.mod Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
pkg/eosclient/eosgrpc/eosgrpc.go Show resolved Hide resolved
pkg/eosclient/eosgrpc/eosgrpc.go Outdated Show resolved Hide resolved
pkg/eosclient/eosgrpc/eosgrpc.go Show resolved Hide resolved
pkg/eosclient/eosgrpc/eosgrpc.go Outdated Show resolved Hide resolved
pkg/eosclient/eosgrpc/eosgrpc.go Outdated Show resolved Hide resolved
pkg/eosclient/eosgrpc/eosgrpc.go Show resolved Hide resolved
pkg/eosclient/eosgrpc/eosgrpc.go Outdated Show resolved Hide resolved
@ffurano
Copy link
Contributor Author

ffurano commented Mar 4, 2021

Hi Ishank, thank for the review. I'll go through all these points in the next days

@lgtm-com
Copy link

lgtm-com bot commented Mar 8, 2021

This pull request introduces 2 alerts when merging 2f5a2e1 into 2011cb5 - view on LGTM.com

new alerts:

  • 2 for Redundant check for negative value

@lgtm-com
Copy link

lgtm-com bot commented Mar 9, 2021

This pull request introduces 2 alerts when merging 7724aea into 491455c - view on LGTM.com

new alerts:

  • 2 for Redundant check for negative value

@hound hound bot deleted a comment from ffurano Mar 12, 2021
@hound hound bot deleted a comment from ffurano Mar 12, 2021
@lgtm-com
Copy link

lgtm-com bot commented Mar 12, 2021

This pull request introduces 2 alerts when merging b6646ac into 1808b1c - view on LGTM.com

new alerts:

  • 2 for Redundant check for negative value

@lgtm-com
Copy link

lgtm-com bot commented Mar 12, 2021

This pull request introduces 2 alerts when merging df43e99 into 1808b1c - view on LGTM.com

new alerts:

  • 2 for Redundant check for negative value

@lgtm-com
Copy link

lgtm-com bot commented Mar 15, 2021

This pull request introduces 2 alerts when merging c0231a7 into 3b7e1f0 - view on LGTM.com

new alerts:

  • 2 for Redundant check for negative value

@lgtm-com
Copy link

lgtm-com bot commented Mar 19, 2021

This pull request introduces 2 alerts when merging 9423a36 into 03346dd - view on LGTM.com

new alerts:

  • 2 for Redundant check for negative value

@lgtm-com
Copy link

lgtm-com bot commented Mar 19, 2021

This pull request introduces 2 alerts when merging 91cb5c4 into 03346dd - view on LGTM.com

new alerts:

  • 2 for Redundant check for negative value

@lgtm-com
Copy link

lgtm-com bot commented Mar 19, 2021

This pull request introduces 2 alerts when merging f2e244e into 03346dd - view on LGTM.com

new alerts:

  • 2 for Redundant check for negative value

@lgtm-com
Copy link

lgtm-com bot commented Mar 29, 2021

This pull request introduces 1 alert when merging e838b6e into e8a00d9 - view on LGTM.com

new alerts:

  • 1 for Useless assignment to local variable

@ishank011
Copy link
Contributor

@ffurano can you rebase instead of merging? It's impossible to review

@ffurano
Copy link
Contributor Author

ffurano commented May 21, 2021

@ffurano can you rebase instead of merging? It's impossible to review

Uhm... I only did rebase so far

@ffurano
Copy link
Contributor Author

ffurano commented May 21, 2021

@ffurano can you rebase instead of merging? It's impossible to review

Uhm... I only did rebase so far

Actually I have the impression that you have put into master some previous stage of my work. I am reviewing all the new conflicts, yet now it's really a mess

@ffurano
Copy link
Contributor Author

ffurano commented May 21, 2021

@ffurano can you rebase instead of merging? It's impossible to review

Rebase done

@lgtm-com
Copy link

lgtm-com bot commented May 27, 2021

This pull request introduces 1 alert when merging 328a1da into 1c0c24a - view on LGTM.com

new alerts:

  • 1 for Missing error check

@glpatcern glpatcern changed the title [WIP] EOS Grpc interface EOS GRPC interface Jun 4, 2021
@labkode
Copy link
Member

labkode commented Jun 4, 2021

@ffurano I see a dependency to a private repo of yours:

github.com/ffurano/grpc-proto

Could you replace it with an upstream version?

Copy link
Member

@labkode labkode left a comment

Choose a reason for hiding this comment

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

Remove private repos

@ffurano
Copy link
Contributor Author

ffurano commented Jun 5, 2021

@ffurano I see a dependency to a private repo of yours:

github.com/ffurano/grpc-proto

Could you replace it with an upstream version?

There's no such a thing by now...

@ffurano
Copy link
Contributor Author

ffurano commented Jun 11, 2021

If I keep it in the Client class then it will leak. It has to be unique in the whole app

Don't we have just one instance of the client struct (not the HTTP client, the one defined in eosgrpc) in the whole app?

Uhm, nothing enforces that in the Client, and it seems pretty awkward to assume that there is just one instance.

The creator can create as many clients it needs, as it goes through the config file:
func New(opt *Options) *Client

@ffurano
Copy link
Contributor Author

ffurano commented Jun 11, 2021

If you have a better way to implement a singleton in golang please let me know... so far I have done more or less like this: https://medium.com/@ishagirdhar/singleton-pattern-in-golang-9f60d7fdab23

@labkode labkode requested review from labkode and ishank011 June 11, 2021 09:43
Copy link
Contributor

@ishank011 ishank011 left a comment

Choose a reason for hiding this comment

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

I'm not sure this should be a singleton in the first place. We'll have one instance of the eos driver to connect to each of the EOS instances and I don't see any advantage of sharing the transport among them. I'd prefer it to be a part of the Client struct. We also need to increase the defaults and make transport configurable.

// From the basepath and the file path... build an url
func (c *Client) buildFullURL(urlpath, uid, gid string) (string, error) {
s := c.opt.BaseURL
if len(urlpath) > 0 && urlpath[0] != '/' {
Copy link
Contributor

Choose a reason for hiding this comment

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

// I feel safer putting here a check, to prohibit malicious users to
// inject a false uid/gid into the url
// Who knows, maybe it's redundant? Better more than nothing.
p1 := strings.Index(urlpath, "eos.ruid")
Copy link
Contributor

Choose a reason for hiding this comment

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

Certificates: []tls.Certificate{cert},
},
MaxIdleConns: 10,
MaxConnsPerHost: 8,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make these configurable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, of course

resp, err := c.cl.Do(req)

// Let's support redirections... and if we retry we have to retry at the same FST, avoid going back to the MGM
if resp != nil && (resp.StatusCode == 307 || resp.StatusCode == 302) {
Copy link
Contributor

@ishank011 ishank011 Jun 14, 2021

Choose a reason for hiding this comment

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

(resp.StatusCode == http.StatusFound || resp.StatusCode == http.StatusTemporaryRedirect)

@ffurano
Copy link
Contributor Author

ffurano commented Jun 14, 2021

I'm not sure this should be a singleton in the first place. We'll have one instance of the eos driver to connect to each of the EOS instances and I don't see any advantage of sharing the transport among them. I'd prefer it to be a part of the Client struct. We also need to increase the defaults and make transport configurable.

Oh, I think you make a good point here about sharing the transport, I'll have a look if it an eos instance can keep the same transport to cache the conns

@ishank011
Copy link
Contributor

@ffurano GetHTTPCl is called on every PUT and GET call, which creates an instance of *ehttp.Client.
I think here we can store the *ehttp.Client

t, err := htopts.Init()
if err != nil {
panic("Cant't init the EOS http client options")
}
c.httptransport = t

and GetHTTPCl can just return that client.

@ffurano
Copy link
Contributor Author

ffurano commented Jun 17, 2021

@ffurano GetHTTPCl is called on every PUT and GET call, which creates an instance of *ehttp.Client.
I think here we can store the *ehttp.Client

t, err := htopts.Init()
if err != nil {
panic("Cant't init the EOS http client options")
}
c.httptransport = t

and GetHTTPCl can just return that client.

This is how it's meant to work, the creation of a new http client is based on the common instance of the transport class, which caches the real connections behind (plus keepalive, max numbers etc). Instances of the client class are meant to be just local and short lived

@ffurano
Copy link
Contributor Author

ffurano commented Jun 17, 2021

@ffurano GetHTTPCl is called on every PUT and GET call, which creates an instance of *ehttp.Client.
I think here we can store the *ehttp.Client

t, err := htopts.Init()
if err != nil {
panic("Cant't init the EOS http client options")
}
c.httptransport = t

and GetHTTPCl can just return that client.

Ah, and now the transport class is not a singleton anymore, there is one instance per EOS endpoint configured

ishank011
ishank011 previously approved these changes Jun 17, 2021
@ishank011 ishank011 merged commit 692f38f into cs3org:master Jun 17, 2021
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