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

Improve nats-js store #123

Merged
merged 5 commits into from
Nov 29, 2023
Merged

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Oct 18, 2023

We found some small issues in the nats-js store implementation:

  • We ran into concurrent map writes when using natsjs store. The problem is occurring on startup when multiple go-routines try to initialize the store at the same time. We reused the existing sync.RWMutex of the store to initialize the store only once.
  • Uses a sync.Map now to store the buckets in a thread safe way
  • Fixes a panic when Limit is larger then the actual slice length on List
  • Trims the prefix from the key when using List. Reason is that other calls (delete, read, write) add the prefix again and modifying records after listing should be possible without further modifying of the keys

@kobergj kobergj mentioned this pull request Oct 18, 2023
3 tasks
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj marked this pull request as draft October 20, 2023 07:26
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj marked this pull request as ready for review October 20, 2023 13:17
@kobergj
Copy link
Contributor Author

kobergj commented Oct 30, 2023

@jochumdev can I get a review on this?

Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj changed the title Avoid concurrent mapwrites in natsjs store Improve nats-js store Nov 28, 2023
@jochumdev
Copy link
Contributor

Regarding sync.Map you might wanna have a look at cornelk/hashmap if speed matters.

I'm using hashmap in go-orb (local fork) and another small project, works fine.

@jochumdev jochumdev self-assigned this Nov 28, 2023
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj
Copy link
Contributor Author

kobergj commented Nov 29, 2023

Thanks for the pointer 👍 Looks much better with hashmap pkg

Copy link
Contributor

@jochumdev jochumdev left a comment

Choose a reason for hiding this comment

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

Nice work!

@jochumdev jochumdev merged commit d72facc into micro:main Nov 29, 2023
2 of 3 checks passed
@kobergj kobergj deleted the NatsjsConcurrentMapWrites branch November 29, 2023 14:32
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.

2 participants