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

Enable using gRPC client for GCS #92

Merged
merged 4 commits into from
Dec 20, 2023

Conversation

fpetkovski
Copy link
Contributor

@fpetkovski fpetkovski commented Dec 6, 2023

GCP has an experimental feature which allows using gRPC when talking to GCS. For the time being, it requires buckets to be explicitly added to an allowlist, but eventually the feature will become GA.

This commit allows using a gRPC client when creating a GCS bucket by toggling a boolean flag in the bucket config.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

  • Allow using gRPC for GCS buckets.

Verification

GCP has an experimental feature which allows using gRPC when talking to GCS.
For the time being, it requires buckets to be explicitly added to an allowlist,
but eventually the feature will become GA.

This commit allows using a gRPC client when creating a GCS bucket by toggling
a boolean flag in the bucket config.

Signed-off-by: Filip Petkovski <[email protected]>
Signed-off-by: Filip Petkovski <[email protected]>
Copy link
Contributor

@MichaHoffmann MichaHoffmann left a comment

Choose a reason for hiding this comment

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

lgtm

Signed-off-by: Filip Petkovski <[email protected]>
Bucket string `yaml:"bucket"`
ServiceAccount string `yaml:"service_account"`
UseGRPC bool `yaml:"use_grpc"`
GRPCConnPoolSize int `yaml:"grpc_conn_pool_size"`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a bit of testing by tweaking this parameter up and down. Performance follows a U-shaped curve where using 1 connection is abysmally slow, even slower than HTTP. With a pool size of 512 performance is better, but it seems to be best with 256.

We could consider using that (or some other number) as the default to make sure users do not need to guess in the beginning. We can still keep the value configurable if anyone needs further customization.

Copy link
Contributor Author

@fpetkovski fpetkovski Dec 6, 2023

Choose a reason for hiding this comment

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

Turns out we had to enable direct path as documented here https://cloud.google.com/go/docs/reference/cloud.google.com/go/storage/latest#hdr-Experimental_gRPC_API.

If the application is running within GCP, users may get better performance by enabling DirectPath (enabling requests to skip some proxy steps). To enable, set the environment variable GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS=true and add the following side-effect imports to your application:

import (
    _ "google.golang.org/grpc/balancer/rls"
    _ "google.golang.org/grpc/xds/googledirectpath"
)

This made the connection pool config obsolete (as a matter of fact, anything higher than 1 would hang the client). The config option is still useful when direct path is not enabled though.

Copy link
Member

Choose a reason for hiding this comment

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

We can probably mention the direct path option as a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, updated.

@fpetkovski
Copy link
Contributor Author

@saswatamcode any thoughts on this one?

Copy link
Member

@saswatamcode saswatamcode left a comment

Choose a reason for hiding this comment

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

Thanks! I think this generally looks good!

Bucket string `yaml:"bucket"`
ServiceAccount string `yaml:"service_account"`
UseGRPC bool `yaml:"use_grpc"`
GRPCConnPoolSize int `yaml:"grpc_conn_pool_size"`
Copy link
Member

Choose a reason for hiding this comment

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

We can probably mention the direct path option as a comment?

Signed-off-by: Filip Petkovski <[email protected]>
@saswatamcode saswatamcode merged commit 9f421f2 into thanos-io:main Dec 20, 2023
7 checks passed
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