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 redundant version checks in transport serialisation #4731

Closed
wants to merge 3 commits into from

Conversation

martijnvg
Copy link
Member

Since 1.0 breaks transport serialisation support with 0.90.x, it doesn't make sense to do version checks in the serialisation code. This will clean up the code (writeTo() and readFrom()) and allows ES to start fresh.

@s1monw
Copy link
Contributor

s1monw commented Jan 14, 2014

this looks great - we can pull this in post RC1

} else {
peroclate = new PercolateStats();
}
peroclate = PercolateStats.readPercolateStats(in);
Copy link
Member

Choose a reason for hiding this comment

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

typo! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

will fix :)

@javanna
Copy link
Member

javanna commented Jan 24, 2014

I think we should be able to remove the read/writeTimeout methods in AcknowledgedRequest as well? If you want I can look into this!

@martijnvg
Copy link
Member Author

@javanna Make sense, I'll remove these methods.

Removed AcknowledgedRequest#readTimeout(StreamInput, Version) and AcknowledgedRequest#writeTimeout(StreamOutput, Version)
@martijnvg
Copy link
Member Author

Updated PR to implement Luca's feedback.


}

@Test
public void testDeleteWarmerTimeoutBwComp_Post0906Format() throws Exception {
Copy link
Member

Choose a reason for hiding this comment

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

just nitpicking, maybe we could change the name of the test method that's left, or even remove it? I don't think we need to explicitly test the serialization here anymore...

@javanna
Copy link
Member

javanna commented Jan 24, 2014

I left a small comment, but looks great @martijnvg , thanks!

@martijnvg
Copy link
Member Author

@javanna Thanks for the feedback.

Pushed to master, 1.x and 1.0 branches.

@martijnvg martijnvg closed this Jan 24, 2014
javanna added a commit that referenced this pull request Jan 24, 2014
javanna added a commit that referenced this pull request Jan 24, 2014
javanna added a commit that referenced this pull request Jan 24, 2014
@martijnvg martijnvg deleted the api-cleanup/serialization branch May 18, 2015 23:33
mute pushed a commit to mute/elasticsearch that referenced this pull request Jul 29, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants