new ets

[ruslandoga]

fixes #71

I'm going to leave some comments inline explaining the changes.

[ruslandoga]

Under load it's actually x40+

x15 (according to Benchee) is when Poolboy is not overloaded

https://github.com/ruslandoga/rate_limit

[ruslandoga]

This log is incomplete, I'll add more changes if the approaches in this PR are approved.

[epinault]

So if we are going to make 7.0 , I would like to make another braking change. So maybe let’s just merge in master without bumping the version yet ?

[epinault]

I would love to see a migrating plan in the changelog from config to the supervisor

[ruslandoga]

I searched for Hammer.inspect_bucket on github and it doesn't seem like anyone is using created_at and updated_at values (almost everyone calls this function to get only ms_to_next_bucket). So I think created_at and updated_at can be removed, especially since created_at in ETS backend is incorrect after table pruning (cleanup).

[epinault]

Yes but we don’t know how people use it in private repo ?

[ruslandoga]

I suggest dropping these values and reintroducing them back if someone complains. Personally, I don't see a scenario where they are useful, especially with ETS backend which cleans up the keys and makes created_at inaccurate.

[egeersoz]

@ruslandoga I'd suggest adding a deprecation warning first, and then getting rid of them in the version after. That's how Elixir and core libraries do it.

[ruslandoga]

I'd like to make table name configurable.

[ruslandoga]

It doesn't seem like the project is using :logger, so it can be removed.

[JuneKelly]

Hi! Thanks for opening this PR, and sorry we haven't seen it until now. I'll try to look at this soon, probably over the winter break.

[JuneKelly]

cc @epinault

[epinault]

Wow yes sorry I ll take a look at it this week but same I never got a notification on this either . I ll have to check my settings cause I usually get notified

[epinault]

some of tha doc is still valid here I think like the optionds so we might need to move that to the hammer.ex

[egeersoz]

Thank you very much for working on this @ruslandoga! I have only one minor suggestion.

[egeersoz]

@ruslandoga I'd suggest adding a deprecation warning first, and then getting rid of them in the version after. That's how Elixir and core libraries do it.

[ruslandoga]

@egeersoz @epinault

Thank you for the comments! I'm going to split this PR into multiple non-breaking ones, some to update CI, some to update ETS backend without the breaking changes, and then a few with the breaking changes.

[ruslandoga]

It seems like no matter where I start: removing application.ex and global supervisor, removing global config and app env lookups, removing poolboy, -- it all results in breaking changes every time :)

And every time I end up with something similar to this:

# instead of config, each backend is defined as a module
defmodule MyApp.RateLimit do
  use Hammer, backend: :ets, table: __MODULE__ # that would be the default table
end

defmodule MyApp.AnotherRateLimit do
  use Hammer, backend: Hammer.Backend.Redis, pool: __MODULE__ # that would be the default pool
end

# each backend can be started separately and be configured at start-up
MyApp.RateLimit.start_link(cleanup_period: :timer.seconds(10))
MyApp.AnotherRateLimit.start_link(url: "...", ...)

# module calls remove the need for runtime backend lookups
{:allow, _} = MyApp.RateLimit.hit("some-key", _scale = :timer.seconds(60), _limit = 3)
1 = MyApp.RateLimit.get("some-key", _scale = :timer.seconds(60), _limit = 3)
MyApp.RateLimit.set("some-key", _scale = :timer.seconds(60), 3)
MyApp.RateLimit.wait("some-key", _scale = :timer.seconds(60), _limit = 3) # = Process.sleep(next_bucket())
MyApp.RateLimit.reset("some-key", _scale = :timer.seconds(60)) # = set(...,...,0)
# etc.

So I'm thinking, would it be OK to just resume this PR and implement this version I always end up with?

[epinault]

sure!