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

feat(storage): add direct google access side-effect imports by default #10757

Merged
merged 12 commits into from
Aug 28, 2024

Conversation

frankyn
Copy link
Member

@frankyn frankyn commented Aug 23, 2024

Add side-effect modules for gRPC DirectPath by default with an opt-out build tag: disable_grpc_modules

  • Update Go storage documentation to reflect this change.
  • Include automatic opt-in for GOOGLE_CLOUD_ENABLE_DIRECT_PATH_XDS=true

@frankyn frankyn added status: blocked Resolving the issue is dependent on other work. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Aug 23, 2024
@frankyn frankyn marked this pull request as ready for review August 27, 2024 16:20
@frankyn frankyn requested review from a team as code owners August 27, 2024 16:20
@frankyn frankyn added api: storage Issues related to the Cloud Storage API. and removed status: blocked Resolving the issue is dependent on other work. do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Aug 27, 2024
@frankyn frankyn enabled auto-merge (squash) August 27, 2024 16:20
@frankyn frankyn disabled auto-merge August 27, 2024 16:21
Copy link
Contributor

@tritone tritone left a comment

Choose a reason for hiding this comment

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

Just some docs suggestions, otherwise LGTM. Nice solution!

storage/doc.go Outdated
_ "google.golang.org/grpc/balancer/rls"
_ "google.golang.org/grpc/xds/googledirectpath"
)
If the application is running within GCP, users may get better performance with Direct Google Access (enabling requests to skip some proxy steps).
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's rephrase this; users don't need to opt-in to Direct Google Access anymore so I don't think this is relevant.

How about:

Using the gRPC API inside GCP with a bucket in the same region can allow for Direct Google Access (enabling requests to skip some proxy steps and reducing response latency).

Also, will a warning be emitted if direct path is attempted and fails? If so we should explain this as well.

storage/doc.go Outdated
)
If the application is running within GCP, users may get better performance with Direct Google Access (enabling requests to skip some proxy steps).

Dependencies for Direct Google Access can increase the size of resulting binary by ~16MiB if not used by another dependency and you can opt-out of these dependencies using build tag `disable_grpc_modules`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems a little ambiguous as to who wants to use this build tag. I'd say something like:

Dependencies for the gRPC API may slightly increase the size of binaries for applications depending on this package. If you are not using gRPC, you can use the build tag disable_grpc_modules to opt out of these dependencies and reduce the binary size.

@frankyn frankyn changed the title feat: add directpath modules by default feat(storage): add direct google access side-effect imports by default Aug 28, 2024
@frankyn frankyn merged commit 9ad8324 into main Aug 28, 2024
11 checks passed
@frankyn frankyn deleted the prototype_grpc_dp_removal branch August 28, 2024 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the Cloud Storage API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants