Extract Adapters #18

julik · 2024-02-26T09:14:04Z

This also adds a Redis adapter and a Memory adapter. The Redis adapter is useful since it can be a continuation of Prorate in a way - but with conditional fillup added in.

The memory store is useful in a different way - it can be used as a reference implementation to test other adapters against.
Also make the adapter configurable on the Pecorino module.

Move them into a module and a directory. This will allow us to expand the adapter corpus shortly.

so that the user is not always at the mercy of auto-detection

Rule of 3

skatkov · 2024-03-13T10:04:25Z

@julik would it be possible to break this up into a separate Pull Requests?

julik · 2024-03-13T10:48:44Z

It is possible, but how useful is it to do so? And where would the separation between the pull requests be, if there are dependencies between changed components?

julik · 2024-03-13T11:29:37Z

Coming to think of it - I can take out the Redis adapter into a separate PR, but not much else 🤔 will that work for you?

lib/pecorino.rb

skatkov · 2024-03-13T15:23:49Z

lib/pecorino/leaky_bucket.rb

@@ -90,12 +90,14 @@ def accepted?
  #   the bucket contents will then be capped at this value. So with
  #   bucket_capacity set to 12 and a `fillup(14)` the bucket will reach the level
  #   of 12, and will then immediately start leaking again.
-  def initialize(key:, capacity:, leak_rate: nil, over_time: nil)
+  # @param adapter[Pecorino::Adapters::BaseAdapter] a compatible adapter
+  def initialize(key:, capacity:, adapter: Pecorino.adapter, leak_rate: nil, over_time: nil)


question: a question not related to this PR, more of a general question for LeakyBucket. I think our previous implementation allowed LeakyBucket to overfill, but you've fixed it, and now it doesn't do that any more.

If value is higher than capacity, it will refuse to fill a bucket.

I can imagine situations, where it would be great if leaky bucket could overfill. For example, if we're implementing a leaky bucket for API rate limiting, you might allow a user to exceed their rate limit by a small margin and then only enforce the limit if they continue to exceed it. This gives a bit of leeway for users who occasionally go over their limits but still protects your system from being overwhelmed by too many requests.

A bucket can never overfill because all fillups get clamped to the capacity value. We have 2 methods - one for conditional fillup (where if your fillup "would" make the bucket overflow it gets refused) and one for unconditional fillup - it will only tell you whether the bucket did spillover as a result of your call. Having these two allows us to implement what you are describing, no? Or I am missing something and "Exceeding by a small margin" needs more information.

unconditional fillup - it will only tell you whether the bucket did spillover as a result of your call.

This is precisely what I was looking for.

lib/pecorino/adapters/redis_adapter.rb

skatkov · 2024-03-13T15:31:03Z

test/adapters/adapter_test_methods.rb

+    sleep 0.65
+
+    # Both the leaky bucket and the block should have expired by now, and `prune` should not raise
+    adapter.prune


nitpick: we don't assert anything in this test. Is this is intentional? What's the point then to have it?

There is a whole discussion about assert_nothing_raised here https://stackoverflow.com/questions/12499259/why-doesnt-minitestspec-have-a-wont-raise-assertion - basically, Ryan posits that if your call raises something, it means the test will fail anyway (and using "assert_nothing_raised" just inflates your green dot count and gives you fake satisfaction for no reason, which is not to be had on these premises!)

That's why I left out the assertion (I think it comes with the activesupport methods, which we do not import into the test suite just now). But I could add it if you think just "calling the thing" is not telling the reader what we are doing. Or we could stick in a comment?..

How do you feel about assert adapter.prune?

Can do - we would need the prune method to return a truthy value for this.

lib/pecorino/adapters/memory_adapter.rb

skatkov · 2024-03-13T15:48:31Z

lib/pecorino/adapters/base_adapter.rb

+  def set_block(key:, block_for:)
+  end
+
+  # Returns the time until which a block for a given key is in effect. If there is no block in


nitpick: it's cool that you're writting these comments, but would it be better to have yard definitions here instead?

Why not both? 😆 good one

skatkov · 2024-03-13T15:51:56Z

README.md

+
+* A memory store (good enough if you have just 1 process)
+* A PostgreSQL or SQLite database (at the moment there is no MySQL support, we would be delighted if you could add it)
+* A Redis instance



nitpick: might be a good idea to explain how to redefine adapter - in case of redis, as example, adapter will not be inherited from ActiveRecord and requires a manual configuration.

None of them inherit from ActiveRecord anymore, some just use the ActiveRecord classes as means of passing connection configuration (and to piggyback on escaping)

None of them inherit from ActiveRecord anymore

Why are you saying that? there is a default_adapter_from_main_database method still defined.

https://github.com/cheddar-me/pecorino/pull/18/files#diff-3ae95d36dd505f2d90c28bab102aceb7c117caee072c2a5910685a35fb1f777bR49

But it would be great to add a section about how to configure adapter for pecorino and which settings are being configured by default.

There is that method indeed (to let users have their adapter picked automatically), but nothing inherits anything there?..

Sorry for the confusion, indeed wrong use of the word "inherits".

skatkov · 2024-03-13T15:57:57Z

@julik checking almost 1000 line of code was a bit overwhelming, but I did my best :)

skatkov · 2024-03-14T11:54:53Z

lib/pecorino/adapters/postgres_adapter.rb

+  end
+
+  def create_tables(active_record_schema)
+    active_record_schema.create_table :pecorino_leaky_buckets, id: :uuid do |t|


suggestion: would it be possible not to force :uuid as a primary key on users? Make it configurable?

Yeah let's switch to whatever ID structure that is the app defalut 👍

skatkov · 2024-03-14T11:56:09Z

lib/pecorino/adapters/postgres_adapter.rb

+    active_record_schema.add_index :pecorino_leaky_buckets, [:key], unique: true
+    active_record_schema.add_index :pecorino_leaky_buckets, [:may_be_deleted_after]
+
+    active_record_schema.create_table :pecorino_blocks, id: :uuid do |t|


suggestion: Same suggestion here -- would be great not to force :uuid as a primary key on users and make this configurable.

lib/pecorino/adapters/base_adapter.rb

skatkov · 2024-03-14T22:44:46Z

lib/pecorino/adapters/redis_adapter.rb

+
+require_relative "base_adapter"
+require "digest"
+require "redis"


Would be great to surround this require with begin rescue. This would help us to throw a meaningful error, if redis gem is missing in gemfile.

begin require "redis" rescue LoadError puts "Please include redis gem in Gemfile" exit end

skatkov · 2024-03-18T13:29:01Z

README.md

+
+* A memory store (good enough if you have just 1 process)
+* A PostgreSQL or SQLite database (at the moment there is no MySQL support, we would be delighted if you could add it)
+* A Redis instance



Sorry for the confusion, indeed wrong use of the word "inherits".

julik added 30 commits March 12, 2024 22:03

Rework how adapters are stored

6ba0cc7

Move them into a module and a directory. This will allow us to expand the adapter corpus shortly.

Start extracting adapter tests

4b63934

Add a memory adapter

8aa5984

Allow multiple adapters to be instantiated

a43958e

Slowly moving on

038f2be

We need some threading tests but this is not "it"

48a22d7

Add stubs for locking in memory adapter

cf75a16

Extract lock

4ae93dd

Rename test

349d38a

Rename leaky bucket test

83568dc

Getting along

fe231c7

Even NOW() is different

6c0e529

Seems to work. Mostly.

eccd557

And one more done

1d67a32

Disallow block_for <= 0

c84453f

Use memory adapter for cached throttle test

bf36b41

Script comment

1e706d1

Tweak a little

c064c81

Still getting there

17eeb81

That mostly works

6c2d7f3

A bit more Volkswagening

da9daff

Make Pecorino.adapter pluggable

c824e29

so that the user is not always at the mercy of auto-detection

Structure tests a bit better still

ff8ee92

Add Redis in CI

4356734

Reformat

cbae5f0

Remove unneeded lint step

145258a

Just on push is sufficient

6087ba9

Document Redis is available now

6e5c6e9

Zap DatabaseAdapter

e8298a7

Rule of 3

Remove database from block_test.rb

6d4fc1b

julik added 2 commits March 12, 2024 22:58

Allow negative values for Block.set!

538842a

Bump version and changelog

5a4bb2b

julik force-pushed the extract-adapters-better branch from 99ab0f8 to 5a4bb2b Compare March 12, 2024 22:00

julik changed the title ~~A better extraction for Adapters~~ Extract Adapters Mar 12, 2024

julik requested a review from skatkov March 12, 2024 22:07

julik marked this pull request as ready for review March 12, 2024 22:07

skatkov reviewed Mar 13, 2024

View reviewed changes

skatkov reviewed Mar 14, 2024

View reviewed changes

lib/pecorino/adapters/base_adapter.rb Show resolved Hide resolved

skatkov reviewed Mar 18, 2024

View reviewed changes

julik added 4 commits March 20, 2024 12:30

Add YARD comments to BaseAdapter

3898531

Better pruning assertions

73f6be5

Document adapter= and adapter

9662a33

Improve memory store cleanup

047c9f0

julik merged commit 65a65af into main Apr 6, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract Adapters #18

Extract Adapters #18

julik commented Feb 26, 2024

skatkov commented Mar 13, 2024

julik commented Mar 13, 2024

julik commented Mar 13, 2024

skatkov Mar 13, 2024

julik Mar 13, 2024

skatkov Mar 14, 2024

skatkov Mar 13, 2024

julik Mar 13, 2024

skatkov Mar 13, 2024

julik Mar 14, 2024

skatkov Mar 13, 2024

julik Mar 13, 2024

skatkov Mar 13, 2024

julik Mar 13, 2024

skatkov Mar 14, 2024

julik Mar 15, 2024

skatkov Mar 18, 2024

skatkov commented Mar 13, 2024

skatkov Mar 14, 2024

julik Mar 15, 2024

skatkov Mar 14, 2024

skatkov Mar 14, 2024

skatkov Mar 18, 2024

Extract Adapters #18

Extract Adapters #18

Conversation

julik commented Feb 26, 2024

skatkov commented Mar 13, 2024

julik commented Mar 13, 2024

julik commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skatkov commented Mar 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment