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

Populate*Options cleanups #13361

Closed
dbolduc opened this issue Dec 26, 2023 · 2 comments
Closed

Populate*Options cleanups #13361

dbolduc opened this issue Dec 26, 2023 · 2 comments
Assignees
Labels
type: cleanup An internal cleanup or hygiene concern.

Comments

@dbolduc
Copy link
Member

dbolduc commented Dec 26, 2023

Some weirdness here

  • The documentation for PopulateGrpcOptions is wrong. It talks about common options, not gRPC options.
  • The REST layers populate gRPC options. This is harmless, just odd.
  • The REST layers default REST options in the stub factory.
  • rest_internal::LongrunningEndpointOption is an internal option? Should customers be able to set this? It is only ever used for gRPC transcoding in the spanner admin rest stubs. (Most REST services use their own endpoint for their own custom LROs).
  • PopulateCommonOptions should really be setting the UnifiedCredentialOption. But it is handled differently in gRPC vs. REST. In gRPC, we default the GrpcCredentialOption. REST defaults the UnifiedCredentialOption and uses MakeAuthOptions(...). I think we can use one code path for both.
  • We do not respect GOOGLE_CLOUD_CPP_TRACING_OPTIONS for RestTracingOptionsOption.

  • RestTracingOptionsOption and GrpcTracingOptionsOption are the same thing. There should only be one type in common. There is a separate proposal on how to resolve this. It is out of scope for this issue.
@dbolduc
Copy link
Member Author

dbolduc commented Dec 27, 2023

PopulateCommonOptions should really be setting the UnifiedCredentialOption

Note to self: We can't do this. We need to maintain backwards compatibility with GrpcCredentialOption, which does not exist in common. In PopulateGrpcOptions we cannot know whether the UnifiedCredentialOption was set by the application or by the library as a default. So we cannot know whether we should use that or GrpcCredentialOption when both are set.

It might be a good idea to compose the gRPC / REST default functions. Such as:

Options PopulateGrpcOptions(Options o, ...) {
  if (!o.has<GrpcOption>()) o.set<GrpcOption>("default");
  o = PopulateCommonOptions(std::move(o), ...);
  return o;
}

@dbolduc
Copy link
Member Author

dbolduc commented Jan 5, 2024

I am closing this. Defaulting REST options is still weird, but I have tasks to do that have deadlines. This is not one of them.

@dbolduc dbolduc closed this as completed Jan 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: cleanup An internal cleanup or hygiene concern.
Projects
None yet
Development

No branches or pull requests

1 participant