-
-
Notifications
You must be signed in to change notification settings - Fork 624
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 😄
Please don't do it by just changing all 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 If you don't want to add mutex first, we still should change internal implment of |
@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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏
This PR completely removes and replaces our native
omap
package with a 3rd-party one. There are a few reasons for this change: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 ofTasks
,Includes
andVars
. This falls inline with the thoughts of the creator of theorderedmap
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 Gomap
does not have built-in concurrency protection.As for the code, I chose elliotchance/orderedmap for a couple of reason over other packages:
O(1)
overSet
,Get
,Delete
andLen
operations.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 theorderedmap
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.