Skip to content

Conversation

@glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Nov 30, 2023

Motivation:

Clients are the highest level API to make requests to a gRPC server.

Modifications:

Add a GRPCClient

Result:

We now have a high level client that can be used by the generated stubs to start RPCs to gRPC servers.

@glbrntt
Copy link
Collaborator Author

glbrntt commented Nov 30, 2023

This builds on top of and supersedes #1727

@glbrntt glbrntt added the version/v2 Relates to v2 label Nov 30, 2023
@glbrntt glbrntt requested a review from FranzBusch November 30, 2023 17:33
/// In contrast to ``RPCError``, the ``ClientError`` represents errors which happen at a scope
/// wider than an individual RPC. For example, attempting to start a client which is already
/// stopped would result in a ``ClientError``.
public struct ClientError: Error, Hashable, @unchecked Sendable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: Should we name this GRPCClientError if the type is called GRPCClient?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, this can be thrown by any client side code, not necessarily the GRPCClient. There are a few places where I'd like to reuse this (one example being an interceptor throwing an error)

private let transport: any ClientTransport

/// The configuration used by the client.
public let configuration: Configuration
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to make this public. Users cannot change anything anymore on it. Do you have a use-case in mind where people might want to access this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Visibility, i.e. being able to see what the configuration is.

///
/// - Parameters:
/// - descriptor: The ``MethodDescriptor`` for which to get or set a ``MethodConfiguration``.
public subscript(_ descriptor: MethodDescriptor) -> MethodConfiguration? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it make sense to subscript for a service in the future? Then I think making the subscript argument label external makes sense.

Suggested change
public subscript(_ descriptor: MethodDescriptor) -> MethodConfiguration? {
public subscript(descriptor: MethodDescriptor) -> MethodConfiguration? {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think so.

If we did eventually want to add one then there's no harm in the service subscript having an external label and this one not having one.

@glbrntt glbrntt merged commit 39c2f4f into grpc:main Dec 4, 2023
@glbrntt glbrntt deleted the gb-client-object branch December 4, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

version/v2 Relates to v2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants