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

Tests: replacement test for missing elements in the diff-serialized cluster state #11257

Closed
imotov opened this issue May 20, 2015 · 5 comments
Closed
Labels
discuss :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one.

Comments

@imotov
Copy link
Contributor

imotov commented May 20, 2015

The commit fd1954d invalidated the assumption that if two cluster states are equal they must have the same binary serialized length. As a result our smoke test for cluster state diffs started to fail periodically and was removed by the commit 42ad677. Therefore now we don't really have a test for a partially implemented elements in cluster state. In other words if a cluster state element is added to writeTo() but we forget to add it to equalTo and we don't json serialize it, we might miss this fact in this test. We need to come up with a better way to catch situations like this.

@jpountz
Copy link
Contributor

jpountz commented May 21, 2015

I have plans to remove block compression as part of #11279 which means that at least we should again get the same length of serialized cluster states. So if it gets in maybe it would be easiest to add the assertion back instead of working on other ways to compare cluster states.

@jpountz
Copy link
Contributor

jpountz commented Jun 10, 2015

@imotov Given recent changes we made to how we perform compression, this assertion should be less of an issue now. Should we just add it back?

@imotov
Copy link
Contributor Author

imotov commented Jun 10, 2015

Yes, it would be great to add it back! Thanks!

@clintongormley
Copy link

@jpountz @imotov is this fixed by #11279?

@imotov
Copy link
Contributor Author

imotov commented Jan 18, 2016

@clintongormley Hmm, I don't think we ever added it back.

imotov added a commit to imotov/elasticsearch that referenced this issue Mar 16, 2016
…state

We can add it back now that we improved our compression framework.

Closes elastic#11257
@clintongormley clintongormley added :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. and removed :Cluster labels Feb 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one.
Projects
None yet
Development

No branches or pull requests

3 participants