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

Add remote peer info to the server context #2136

Merged
merged 2 commits into from
Dec 3, 2024

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Dec 2, 2024

Motivation:

It's often useful to know the identity of the remote peer when handling RPCs.

Modifications:

  • Add a 'peer' to the server context
  • Implement this for the in-process transport
  • Make some in-process inits package, these should never have been public

Result:

Server RPCs have some idea what the address of remote peer is

Motivation:

It's often useful to know the identity of the remote peer when handling
RPCs.

Modifications:

- Add a 'peer' to the server context
- Implement this for the in-process transport

Result:

Server RPCs have some idea what the address of remote peer is
@glbrntt glbrntt added the ⚠️ semver/major Breaks existing public API. label Dec 2, 2024
@glbrntt
Copy link
Collaborator Author

glbrntt commented Dec 2, 2024

API breakages is expected.

@@ -47,13 +47,13 @@ struct ClientRPCExecutorTestHarness {

switch transport {
case .inProcess:
let server = InProcessTransport.Server()
let server = InProcessTransport.Server(peer: "in-process")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth adding a pid to the test transports? Even adding something like :1234 might come in useful in the future just so formatting expectations are upheld.

@glbrntt glbrntt merged commit 0fc4956 into grpc:main Dec 3, 2024
42 of 45 checks passed
@glbrntt glbrntt deleted the v2/remote-peer-info branch December 3, 2024 13:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants