Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
e42be42 to
30a1067
Compare
30a1067 to
e1bb5fb
Compare
c910ed3 to
3fb2567
Compare
3fb2567 to
daaf68a
Compare
daaf68a to
7ca783c
Compare
7ca783c to
9905b6e
Compare
9905b6e to
66afa95
Compare
66afa95 to
26d0fde
Compare
26d0fde to
136e9ce
Compare
136e9ce to
1ddd21b
Compare
1ddd21b to
08ecb13
Compare
4d9fb44 to
c77488e
Compare
c77488e to
b768584
Compare
b768584 to
db79f9b
Compare
| this.resumableUploadStub = null; | ||
| } | ||
|
|
||
| if (resumableUploadStub != null) { |
There was a problem hiding this comment.
resumableUploadStub is only generated if resumable upload methods exist, so I don't think we need this null check.
There was a problem hiding this comment.
I reworked this to look a bit less awkward - but the stub can also be null if the user initialized the client with a gRPC channel and credentials can't be extracted (so that init succeeds but calls to the resumable upload stub fail fast).
There was a problem hiding this comment.
It is a possible scenario but I'm not sure how we can detect that actually. Customers can pass a FixedChannelProvider with their custom gRPC channel and credentials. In this case, we would still create a resumable upload stub with a default credential provider, but it would not work. I think we may have to let it fail during runtime.
There was a problem hiding this comment.
Hmmm, yeah I see that the current check is insufficient. I don't think we can just let calls fail at runtime though; we have a requirement that "when a gRPC client surface for a resumable upload API is initialized with a pre-constructed gRPC Channel, the Client Libraries must raise a detailed (actionable) error if a resumable method is called". So we need to be able to distinguish between this case and other failures.
WDYT about switching the check to be if (settings.getTransportChannelProvider() instanceof InstantiatingGrpcChannelProvider), is that sufficient to detect the custom channel or are there gotchas about that way too?
It also wouldn't necessarily have to be here at the top level; it could get pushed into HttpJson<Service>ResumableUploadStub.create(...) and set the callable there to throw with the actionable error. Then the specialized stub could be always non-null for services that need resumable uploads.
There was a problem hiding this comment.
WDYT about switching the check to be if (settings.getTransportChannelProvider() instanceof InstantiatingGrpcChannelProvider), is that sufficient to detect the custom channel or are there gotchas about that way too?
The default grpc channel is also InstantiatingGrpcChannelProvider.
we have a requirement that "when a gRPC client surface for a resumable upload API is initialized with a pre-constructed gRPC Channel, the Client Libraries must raise a detailed (actionable) error if a resumable method is called"
The way I interpreted it is that we should be good as long as we surface the runtime error (likely an auth error) to the customer. It does not have to be an error before making the call.
There was a problem hiding this comment.
DIscussed offline - we will pause on detecting the custom gRPC channel and unconditionally create the specialized stub (so now it's always non-null). This PR is now in that state.
We clarified that it is still a requirement to provide a more actionable error than bubbling up whatever may happen to fail when a resumable upload method is called if the client library was initialized with a gRPC channel, but we will defer adding that functionality for now.
|
|
||
| if (clientContext.getCredentials() != null) { | ||
| this.resumableUploadStub = | ||
| HttpJsonResumableUploadServiceResumableUploadStub.create(clientContext, settings); |
There was a problem hiding this comment.
As I mentioned in another comment, we might be able to just pass the settings to HttpJsonResumableUploadServiceResumableUploadStub.
There was a problem hiding this comment.
Responded in more detail on that thread - I'm not sure what the best tradeoff there is
There was a problem hiding this comment.
This is now using the settings directly
db79f9b to
627610b
Compare
627610b to
6341e4e
Compare
378b601 to
0416f83
Compare
0416f83 to
1b3fd06
Compare
1b3fd06 to
ba9463b
Compare
443d75f to
2ffa0af
Compare
…tubs Wires the generated transport stubs (GrpcServiceStub and HttpJsonServiceStub) to delegate resumable upload methods to the internal HTTP upload stub. In GrpcServiceStub, credentials, headers, and clocks are forwarded to the underlying HTTP client context. Clients initialize successfully without credentials; an exception results only if use of the HTTP upload stub is attempted. Also declares the stub-side contract that the transports implement: - In AbstractServiceStubClassComposer: emit public ResumableUploadCallable<RequestT, ResponseT> [method]Callable() throwing UnsupportedOperationException.
2ffa0af to
c08a674
Compare
|
|
2 participants
Wires generated transport stubs (
GrpcServiceStub,HttpJsonServiceStub) to delegate resumable upload methods to the internal HTTP upload stub, and excludes upload RPCs from the main stubs' method descriptors and callables.