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

Tidy up Netty4 HTTP response interfaces #104675

Merged

Conversation

DaveCTurner
Copy link
Contributor

Renames Netty4RestResponse to Netty4HttpResponse and removes some
unused super-interfaces and methods.

Renames `Netty4RestResponse` to `Netty4HttpResponse` and removes some
unused super-interfaces and methods.
@DaveCTurner DaveCTurner added >non-issue :Distributed Coordination/Network Http and internode communication implementations v8.13.0 labels Jan 24, 2024
@DaveCTurner DaveCTurner requested a review from ywangd January 24, 2024 08:27
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@elasticsearchmachine elasticsearchmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Jan 24, 2024
Copy link
Member

@ywangd ywangd left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +43 to +51
@Override
public void addHeader(String name, String value) {
headers().add(name, value);
}

@Override
public boolean containsHeader(String name) {
return headers().contains(name);
}
Copy link
Member

Choose a reason for hiding this comment

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

Personally I feel it is natural for Netty4HttpResponse to extend HttpResponse so that these methods can be kept at interface level. Or we should have a comment somewhere to explain why future refactor should not make such change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm currently working on a (possible) enhancement that would allow us to split responses over multiple such messages, for which it only makes sense for the first message in the sequence to extend HttpResponse and carry stuff like headers. If that lands then this'll explain why we've cut down this interface, and if it doesn't then we can pull these back up to the super-interface.

@DaveCTurner DaveCTurner merged commit 6a0dba6 into elastic:main Jan 24, 2024
15 checks passed
@DaveCTurner DaveCTurner deleted the 2024/01/24/Netty4HttpResponse-interface branch January 24, 2024 10:00
@DaveCTurner
Copy link
Contributor Author

FWIW this was opened to prepare things for #104851.

@DaveCTurner DaveCTurner restored the 2024/01/24/Netty4HttpResponse-interface branch June 17, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Coordination/Network Http and internode communication implementations >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v8.13.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants