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

Memory Usage #341

Merged
merged 10 commits into from
Sep 4, 2023
Merged

Memory Usage #341

merged 10 commits into from
Sep 4, 2023

Conversation

davidroman0O
Copy link
Contributor

It won't be 1:1 to Redis since it's adding overhead.

MEMORY USAGE foo

It's quite simple and using a dependency that got one file.

Story time: I was experimenting with miniredis + asynq + rueidis all on the same codebase just for fun, when i wanted to call GetTaskInfo of asynq which lead me to this lua code that was throwing an error because miniredis was missing MEMORY USAGE. Using miniredis just like i would use sqlite is just awesome.

Please consider allowing sponsoring, I like to donate a bit for the code I use for my side projects :)

This PR will help asynq even though the maintainer doesn't even know about this case

@alicebob
Copy link
Owner

Hi,

thanks for the PR and the kind words. I prefer to not have a dependency for something used this rare, but since the license is MIT we could add the file to the codebase (same as geohash and hyperloglog). Just that one file and the license+credits, I guess.
And could you add a small test in ./integration? There's a DoLoosly() which compares the returned structure only and not the exact values.

After re-reading the doc and re-trying on my side on different types, indeed we need to do it across everything

I took the suggestion of @alicebob to put the dependency in a folder and credit, I don't know for the polishing required
@davidroman0O
Copy link
Contributor Author

That would do, I guess

I've changed the requirements a little bit since after re-reading the documentation this morning, it require to work on all types so I used the master map to switch on the types. I hope i didn't forgot something.

The size package is within a folder which contains a readme, I don't know how to proceed for the credits though!

I've added two lines in the integration, i don't if it's what you wanted :P

go.exe test -timeout 30s -run ^TestServer$ github.com/alicebob/miniredis/v2/integration
ok  	github.com/alicebob/miniredis/v2/integration	0.192s

@alicebob
Copy link
Owner

Thanks for the fixes. It looks good, but I'll have a proper look soon. Can you squash it all so it's easier to merge? Thanks!

notes

based on master key

After re-reading the doc and re-trying on my side on different types, indeed we need to do it across everything

I took the suggestion of @alicebob to put the dependency in a folder and credit, I don't know for the polishing required

Added small test
@davidroman0O
Copy link
Contributor Author

davidroman0O commented Aug 30, 2023

I think i need to sleep before doing other mistakes

@alicebob alicebob merged commit c9da5b0 into alicebob:master Sep 4, 2023
4 checks passed
@alicebob
Copy link
Owner

alicebob commented Sep 4, 2023

Merged, with some small fixes. I hope you got the credits for the commit. I'll make it a proper release in a few days.

(The tests in ./integration go against a real redis. If you run make in there it works: it will run INT=1 go test. If the tests can't find redis on your system you can run ./get_redis.sh to install it in a subdir there).

@wszaranski wszaranski mentioned this pull request Apr 9, 2024
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