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

Default constructor for MapRef #4136

Open
armanbilge opened this issue Sep 13, 2024 · 4 comments · May be fixed by #4139
Open

Default constructor for MapRef #4136

armanbilge opened this issue Sep 13, 2024 · 4 comments · May be fixed by #4139

Comments

@armanbilge
Copy link
Member

armanbilge commented Sep 13, 2024

We currently have many MapRef implementations, based on Ref[Map], sharding, ConcurrentHashMap, etc. We should have an apply constructor that chooses a reasonable default implementation, probably ConcurrentHashMap.

@yisraelU
Copy link

@armanbilge I'd like to pick this up

@armanbilge
Copy link
Member Author

@BalmungSan raised a good point that this default constructor should only require a Concurrent constraint. We can pattern-match for Async to use ConcurrentHashMap implementation, otherwise maybe we can fallback to the sharded Ref[Map] with the number of shards determined by the number of processors.

@yisraelU yisraelU linked a pull request Sep 17, 2024 that will close this issue
@Jasper-M
Copy link
Contributor

Jasper-M commented Sep 17, 2024

While we're at it, a nicer API for defaultedMapRef would also be welcome. E.g. a constructor that directly takes a default value, or an instance method on trait MapRef. MapRefOptionOps is probably a good place for it too.

@armanbilge
Copy link
Member Author

MapRefOptionOps is probably a good place for it too.

Yes, I think this would be my vote. I think should be easy to chain with the constructor as well so we won't need a separate constructor?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

3 participants