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

Does the bucket name have to be a string? #17

Closed
net opened this issue Jan 5, 2017 · 7 comments
Closed

Does the bucket name have to be a string? #17

net opened this issue Jan 5, 2017 · 7 comments

Comments

@net
Copy link

net commented Jan 5, 2017

I'm using tuples for bucket names and they seem to work fine, however the README and typespecs explicitly say string. I briefly looked through the code and didn't see anything that required bucket names to be strings, but I may have missed something.

If they're not restricted to strings it might be helpful to update the docs and typespecs.

@grempe
Copy link
Owner

grempe commented Jan 10, 2017

Sorry for the delay. I have to state up-front, I'm a little rusty on this since I'm not currently using Elixir/Erlang. However, I think what you are doing may work, but I'm not sure its recommended.

Here is an example of where that is being called:

https://github.com/grempe/ex_rated/blob/master/lib/ex_rated.ex#L215

and the Key is already of the form {bucket_number, id}, your you seem to be replacing the id with your Tuple.

Looking at ETS docs:

https://elixirschool.com/lessons/specifics/ets/
http://erlang.org/doc/man/ets.html#update_counter-3

It looks like the Key can be an Erlang term().

So it may work, but I'm not sure if that is accidental or not. If you want to dig into this some more and submit a PR updating docs and tests to accept that I would certainly take a look. I'm not sure what the benefits are though of doing it this way. Maybe you can elaborate.

@net
Copy link
Author

net commented Jan 10, 2017

If it's just passing it on to ETS then it's certainly allowed. The benefits are many; in my case I'm rate limiting IPs from Phoenix's conn.remote_ip which come in the form {123, 45, 678, 90}. It's simpler and more efficient to just use the tuple instead of converting it to a string.

@grempe
Copy link
Owner

grempe commented Jan 10, 2017

OK, if you want to work up a docs change and add tests to demonstrate I'll be happy to take a look.

@net
Copy link
Author

net commented Jan 10, 2017

Alright.

One other question: are buckets unique by their scale and limit, or just by their name?

@grempe
Copy link
Owner

grempe commented Jan 10, 2017

Well the key is tangled with the scale to some degree in the following function that creates the key, but I would not say the bucket is unique across all those values.

https://github.com/grempe/ex_rated/blob/master/lib/ex_rated.ex#L254

@grempe
Copy link
Owner

grempe commented Dec 27, 2017

I'll mark this as help wanted. If someone would like to add tests including the use of any Erlang term() as a key and modify the README that would be helpful.

denvera added a commit to denvera/ex_rated that referenced this issue Oct 9, 2019
@grempe
Copy link
Owner

grempe commented Jun 23, 2020

PR #35 merged. Thank you @denvera

@grempe grempe closed this as completed Jun 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants