Skip to content

Remove Unused Features Field on StreamOutput #44667

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 23, 2019
Merged

Remove Unused Features Field on StreamOutput #44667

merged 2 commits into from
Jul 23, 2019

Conversation

@original-brownbear
Copy link
Contributor

@original-brownbear original-brownbear commented Jul 21, 2019 •
edited
Loading

  • Ever since b15d62c this field and all the methods around it seem completely unused (that commit removed the only use of the getter) and
    are in fact wasting some allocations => removed it
    • Note: We are writing the features map by a different path for requests, it wouldn't really make any sense to check features in responses anyway when the request contains the supported requests by the sender as far as I can see

@original-brownbear
Remove Unused Features Field on StreamOutput
ff3bbba 
* Ever since b15d62c this field and all the methods around it seem completely unused (that commit removed the only use of the getter) and
are in fact wasting some allocations => removed it
@elasticmachine
Copy link
Collaborator

elasticmachine commented Jul 21, 2019

Pinging @elastic/es-core-infra

original-brownbear
original-brownbear commented Jul 21, 2019
View reviewed changes
server/src/main/java/org/elasticsearch/transport/OutboundMessage.java

@Override
public void writeTo(StreamOutput out) throws IOException {
out.setFeatures(features);
Copy link
Contributor Author

@original-brownbear original-brownbear Jul 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This was (and now would have been even more) a weird writeTo that doesn't actually write anything in case of a response. Adjusted the abstractions a little here and just made the request overwrite the message write code to prefix messages with the features array.

Tim-Brooks
Tim-Brooks approved these changes Jul 22, 2019
View reviewed changes
Copy link
Contributor

@Tim-Brooks Tim-Brooks left a comment

Choose a reason for hiding this comment

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

LGTM

@original-brownbear
Copy link
Contributor Author

original-brownbear commented Jul 23, 2019

thanks @tbrooks8 !

@original-brownbear original-brownbear merged commit 42a331c into elastic:master Jul 23, 2019
@original-brownbear original-brownbear deleted the remove-dead-code-streams branch July 23, 2019 06:15
@jasontedor jasontedor mentioned this pull request Jul 28, 2019
Merged
@original-brownbear original-brownbear restored the remove-dead-code-streams branch August 6, 2020 18:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Tim-Brooks Tim-Brooks Tim-Brooks approved these changes

@jasontedor jasontedor Awaiting requested review from jasontedor

Assignees

No one assigned

Labels

:Core/Infra/Core Core issues without another label >non-issue v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@original-brownbear @elasticmachine @Tim-Brooks @jakelandis