Skip to content

Use new sets only in mango #5510

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

Merged
merged 1 commit into from
Apr 22, 2025
Merged

Use new sets only in mango #5510

merged 1 commit into from
Apr 22, 2025

Conversation

nickva
Copy link
Contributor

@nickva nickva commented Apr 22, 2025

We already use new ones everywhere else and when switching to OTP 28 we got a bunch of failure do to various order changes, so make sure to use new ones everywhere.

Moreover, do not rely on implementation order of sets:to_list(), but sort the result to produce deterministic results regardless of the internal representation.

@nickva nickva mentioned this pull request Apr 22, 2025
Copy link
Contributor

@big-r81 big-r81 left a comment

Choose a reason for hiding this comment

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

+1

We already use new ones everywhere else and when switching to OTP 28 we got a
bunch of failure do to various order changes, so make sure to use new ones
everywhere.

Moreover, do not rely on implementation order of `sets:to_list()`, but sort the
result to produce deterministic results regardless of the internal
representation.
@nickva nickva force-pushed the use-new-sets-only branch from 974280c to 09ef100 Compare April 22, 2025 06:14
@nickva nickva merged commit b04d1f0 into main Apr 22, 2025
23 checks passed
@nickva nickva deleted the use-new-sets-only branch April 22, 2025 07:01
@nickva
Copy link
Contributor Author

nickva commented Apr 22, 2025

This is related to the effort to enable OTP 28 compatibility #5507

@@ -583,6 +585,9 @@ ddoc_name(<<"_design/", Name/binary>>) ->
ddoc_name(Name) ->
Name.

set_from_list(KVs) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Would not it be better to move this definition to some shared utility module instead of duplicating its definition in multiple modules? One could include a similar wrapper for sets:new([{version, 2}]) (as e.g. set_new/0) to extend consistency to the creation of sets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right, it's one line only but it's repeated 4 times, a util function would be nicer here.

nickva added a commit that referenced this pull request Apr 23, 2025
With OTP 28 sets switchd to using version 2 by default. That caused various
tests to fail, as Jan discovered in #5507

In the previous PR #5510 we switched few mango instances to use a helper
utility, but it was repeated a few times and Gábor made a good suggestion to
use utility functions instead, so that's what this PR does. But it does it for
the whole code base.

Once we're post OTP 28 we can remove the utility and just use plain set
functions.
@nickva nickva mentioned this pull request Apr 23, 2025
nickva added a commit that referenced this pull request Apr 23, 2025
With OTP 28 sets switchd to using version 2 by default. That caused various
tests to fail, as Jan discovered in #5507

In the previous PR #5510 we switched few mango instances to use a helper
utility, but it was repeated a few times and Gábor made a good suggestion to
use utility functions instead, so that's what this PR does. But it does it for
the whole code base.

Once we're post OTP 28 we can remove the utility and just use plain set
functions.
nickva added a commit that referenced this pull request Apr 23, 2025
With OTP 28 sets switchd to using version 2 by default. That caused various
tests to fail, as Jan discovered in #5507

In the previous PR #5510 we switched few mango instances to use a helper
utility, but it was repeated a few times and Gábor made a good suggestion to
use utility functions instead, so that's what this PR does. But it does it for
the whole code base.

Once we're post OTP 28 we can remove the utility and just use plain set
functions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants