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

Fix ClusterSearchShardsResponseTests#testSerialization #100557

Merged

Conversation

benwtrent
Copy link
Member

Fixes serialization random versioning.

closes: #100289

@benwtrent benwtrent added >test Issues or PRs that are addressing/adding tests :Search/Search Search-related issues that do not fall into other categories labels Oct 9, 2023
@benwtrent benwtrent requested review from javanna and ldematte October 9, 2023 21:24
@elasticsearchmachine elasticsearchmachine added the Team:Search Meta label for search team label Oct 9, 2023
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-search (Team:Search)

Version nodeVersion = VersionUtils.randomVersion(random());
DiscoveryNodeUtils.Builder node = DiscoveryNodeUtils.builder(shardRouting.currentNodeId())
.address(new TransportAddress(TransportAddress.META_ADDRESS, randomInt(0xFFFF)))
.version(nodeVersion, IndexVersion.getMinimumCompatibleIndexVersion(nodeVersion.id), IndexVersion.fromId(nodeVersion.id));
Copy link
Contributor

@ldematte ldematte Oct 10, 2023

Choose a reason for hiding this comment

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

For nodeVersion > 8.10, I fear that fromId could give you back a IndexVersion you are expecting.
I would use IndexVersionUtils.randomVersion() if you want to introduce additional randomness, or IndexVersionUtils.current() if you want something that covers all.

Nit: I would consider using IndexVersion.ZERO or IndexVersion.MINIMUM_COMPATIBLE for min.

CC @thecoop as he is the expert in this area.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, you can't go from version -> indexversion. Using IndexVersion.MINIMUM_COMPATIBLE and IndexVersion.current() is the best bet here

@benwtrent benwtrent requested a review from ldematte October 10, 2023 11:58
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

LGTM

@benwtrent benwtrent merged commit 0efd101 into elastic:main Oct 10, 2023
@benwtrent benwtrent deleted the test/fix-ClusterSearchShardsResponseTests branch October 10, 2023 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team >test Issues or PRs that are addressing/adding tests v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] ClusterSearchShardsResponseTests testSerialization failing
4 participants