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

feat: use external package for ordered maps #1797

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat: use external package for ordered maps #1797

wants to merge 1 commit into from

Conversation

pd93
Copy link
Member

@pd93 pd93 commented Sep 9, 2024

This PR completely removes and replaces our native omap package with a 3rd-party one. There are a few reasons for this change:

  • It is not very performant compared to other implementations.
  • An internal implementation leads to very bespoke code whereas using a generic package enforces a cleaner separation between the omap implementation detail and Task features.
  • One less thing for us to maintain. reuse > rewrite.

This PR also makes way for the use of iterators (which we cannot use until we bump our minimum version to 1.23). A working PoC of this is available in #1798.

This also has a direct knock-on effect to #1605 - though falls short of fixing it. @trim21 has #1701 open to address this. I'm afraid that this will need rethinking as the fix was in our custom omap implementation previously. Now that we have a bit more separation, we will need to bring the fixes into our implementations of Tasks, Includes and Vars. This falls inline with the thoughts of the creator of the orderedmap package we are moving to. That is, concurrency should be fixed by the implementation, not the ordered map itself. This is similar to how a Go map does not have built-in concurrency protection.


As for the code, I chose elliotchance/orderedmap for a couple of reason over other packages:

  • It maintains O(1) over Set, Get, Delete and Len operations.
  • Supports iterators
  • Good API design
  • Well maintained and reliable author

I have opened a couple of issues on the project that would ease our integration. However, neither of there are dealbreakers - Just nice-to-have:


One additional note. We now use custom unmarshalling on all implementations of ordered maps (previously this was only on Includes). This is because the orderedmap package does not natively support is. It may do in the future, but this doesn't matter to us as the custom unmarshal functions actually tidy up some of our code. Generally speaking, I have tried to tidy up and make all the implementations of ordered maps in our code a bit more consistent.

@pd93 pd93 mentioned this pull request Sep 9, 2024
@pd93 pd93 requested review from vmaerten and andreynering and removed request for vmaerten September 19, 2024 08:41
Copy link
Member

@vmaerten vmaerten left a comment

Choose a reason for hiding this comment

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

Great description 👏 and great PR. It fixes the issue and tidy up our code.
I totally agree with you on those 3 points. Also the author seems to be reactive (as he already answered one of your issue)

Globally happy with this changes.

Looking forward to be able to use the iterator 😄

@trim21
Copy link
Contributor

trim21 commented Sep 20, 2024

Please don't do it by just changing all internal/omap to new package.

This ordered maps doesn't have mutex and it's not goroutine safe, so we still need to add mutex to fix fatal error: concurrent map writes in the future, and that's why we change omap in the first please.

This means we will still need to add mutex to ordered map struct in the future. and it's much easier to add it with internal/omap/.

If you don't want to add mutex first, we still should change internal implment of internal/omap instead of removing it.

@pd93
Copy link
Member Author

pd93 commented Sep 20, 2024

@trim21 I'm working on a separate branch to add the mutexes. I didn't want to make this PR even larger than it already is though.

I want to get both merged before we release again.

@trim21
Copy link
Contributor

trim21 commented Sep 20, 2024

@trim21 I'm working on a separate branch to add the mutexes. I didn't want to make this PR even larger than it already is though.

I want to get both merged before we release again.

swap internal implement of omap will only make it smaller, not bigger.

@pd93
Copy link
Member Author

pd93 commented Sep 20, 2024

swap internal implement of omap will only make it smaller, not bigger.

I mean I don't want to make this PR bigger by adding the mutexes to the implementations now. It will make the refactor and the fix harder to review. I intend to release both at the same time.

I do not want to have an internal omap package for the reasons stated in the PR description.

Copy link
Member

@andreynering andreynering left a comment

Choose a reason for hiding this comment

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

👏

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.

4 participants