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

Remove remoteAddress field from TransportResponse #120016

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

original-brownbear
Copy link
Member

This field is only used (by security) for requests, having it in responses is redundant. Also, we have a couple of responses that are singletons/quasi-enums where setting the value needlessly might introduce some strange contention even though it's a plain store.

This isn't just a cosmetic change. It makes it clear at compile time that each response instance is exclusively defined by the bytes that it is read from. This makes it easier to reason about the validity of suggested optimizations like #120010
Also, we could now make TransportResponse and interface which would enabled some serious code shorting by turning many responses into records.

This field is only used (by security) for requests, having it in responses is redundant.
Also, we have a couple of responses that are singletons/quasi-enums where setting the value
needlessly might introduce some strange contention even though it's a plain store.

This isn't just a cosmetic change. It makes it clear at compile time that each response instance
is exclusively defined by the bytes that it is read from. This makes it easier to reason about the
validity of suggested optimizations like elastic#120010
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/Transport API Transport client API >non-issue Team:Core/Infra Meta label for core/infra team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants