Skip to content

Conversation

@thecoop
Copy link
Member

@thecoop thecoop commented Jan 30, 2025

We can use NavigableSet to represent version sets to show that they are sorted and unique, rather than lists that don't enforce this.

@thecoop thecoop added :Core/Infra/Core Core issues without another label >refactoring labels Jan 30, 2025
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team v9.0.0 labels Jan 30, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra (Team:Core/Infra)

@elasticsearchmachine elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Jan 30, 2025
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.

I skimmed though this and I thing it's a good idea; the only drawbacks I can think of are related to performance (e.g. if you need to traverse all versions), but it seems to me that most of the operations are equivalent (e.g. first, last). Wdyt?

@thecoop
Copy link
Member Author

thecoop commented Jan 30, 2025

Yes, the main change is use of randomFrom implementation using an iterator. This will not be iterating over a lot of items each time, and it's only use is in tests, so I don't think this will have an measurable impact.

We could use lists in some places in test code if you think this will matter, or define some kind of immutable sorted set with a backing list?

@ldematte
Copy link
Contributor

define some kind of immutable sorted set with a backing list?

That's a cool idea, but I don't think it's necessary; like you said, the iteration is limited and performed only by test code

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

I think we only need it to be navigable in tests, right? Could we maintain the sorted List in production code, and have the version util for tests build a TreeSet from that?

@thecoop
Copy link
Member Author

thecoop commented Jan 31, 2025

I've kept a List on the production classes. I've also introduced a logarithmic O(nlogn) algo for randomFrom in navigable sets, which eliminates the sequential iteration over the sets

@thecoop thecoop requested review from ldematte and rjernst January 31, 2025 10:14
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, I like the new random utility!

@thecoop thecoop added the v9.0.0 label Jan 31, 2025
@thecoop thecoop added the auto-backport Automatically create backport pull requests when merged label Jan 31, 2025
@thecoop thecoop merged commit c288684 into elastic:main Feb 3, 2025
16 of 17 checks passed
@thecoop thecoop deleted the version-sets branch February 3, 2025 14:02
thecoop added a commit to thecoop/elasticsearch that referenced this pull request Feb 3, 2025
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
9.0

nielsbauman added a commit to nielsbauman/elasticsearch that referenced this pull request Feb 11, 2025
This failing test was already fixed by elastic#121266

Fixes elastic#121495
nielsbauman added a commit that referenced this pull request Feb 13, 2025
This failing test was already fixed by #121266

Fixes #121495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged :Core/Infra/Core Core issues without another label >refactoring serverless-linked Added by automation, don't add manually Team:Core/Infra Meta label for core/infra team v9.0.0 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants