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.

Sign up for GitHub

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

Jump to bottom

[ETCM-266]-replaced-rate-limiter-built-on-twitter #873

Merged

dmitry-worker merged 15 commits into develop from feature/ETCM-266-no-twitter-util-collection

Jan 1, 2021

Contributor

dmitry-worker commented Dec 23, 2020 •

edited

Loading

Description

Twitter-collections used for LRU cache for Scala 2.13 is unavailable.
Had to replace this LRU by something with similar functionality

Proposed Solution

The simplest possible LRU is built on LinkedHashMap
Boilerplate code is replaced by Directive0 implementation
Further improvement steps offered for JsonRPC server

dmitry-worker requested review from enriquerodbe, lemastero and biandratti

December 23, 2020 14:47


          [ETCM-266]-replaced-rate-limiter-built-on-twitter

cf27856

dmitry-worker force-pushed the feature/ETCM-266-no-twitter-util-collection branch from 2dd8457 to cf27856 Compare

December 23, 2020 14:54

enriquerodbe reviewed

View reviewed changes

src/main/scala/io/iohk/ethereum/db/cache/SimpleLRU.scala Outdated Show resolved Hide resolved

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/RateLimit.scala Outdated Show resolved Hide resolved

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/RateLimit.scala Outdated Show resolved Hide resolved

src/test/scala/io/iohk/ethereum/jsonrpc/server/http/JsonRpcHttpServerSpec.scala Outdated Show resolved Hide resolved

src/test/scala/io/iohk/ethereum/db/cache/SimpleLRUSpec.scala Outdated Show resolved Hide resolved

Dmitry Voronov added 5 commits

December 24, 2020 22:16


          Refactored RateLimit, extracted lazy LRU

b04bc3d


          Formatting applied

f7e0a69


          Scalastyle hanging comma fix

59dc8df


          Updated RateLimit algebra

16bddee


          Updated comments, fixed imports

10fb166

enriquerodbe reviewed

View reviewed changes

src/main/scala/io/iohk/ethereum/db/cache/SimpleLRU.scala Outdated Show resolved Hide resolved

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/RateLimit.scala Outdated Show resolved Hide resolved

biandratti reviewed

View reviewed changes

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/JsonRpcHttpServer.scala Show resolved Hide resolved

Dmitry Voronov added 3 commits

December 29, 2020 14:58


          Fixed test to advance time programmaticaly

d9584ca


          Transit to guava cache

c61ec05


          Gave up on returning function value in RL algebra

405a76a

dmitry-worker requested a review from enriquerodbe

December 29, 2020 17:46

enriquerodbe approved these changes

View reviewed changes

biandratti reviewed

View reviewed changes

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/RateLimit.scala Show resolved Hide resolved

biandratti reviewed

View reviewed changes

src/test/scala/io/iohk/ethereum/jsonrpc/server/http/JsonRpcHttpServerSpec.scala Show resolved Hide resolved

lemastero reviewed

View reviewed changes

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/RateLimit.scala Outdated

+                          }
+                        )
+                        if (exists) {
+                          val err = JsonRpcError.RateLimitError(minInterval)

Contributor

lemastero Dec 30, 2020

Can we separate LRU cache from RateLimit'ing?

I believe there are some rate-limiting capabilities built-in cats or Akka maybe we could use sth that is already build in?

Contributor Author

dmitry-worker Dec 30, 2020

I'm not sure I can get the idea of separating.
Do you mean an ability to plug an opt-in rate limiting engine?

Dmitry Voronov added 5 commits

December 30, 2020 17:03


          fixed repo.nix

202f5ac


          Formatting fixed

ec001ba


          Extracted blocker method


          Scalastyle hanging comma fix v2

4df7be8


          Merge branch 'develop' into feature/ETCM-266-no-twitter-util-collection

711867d

biandratti reviewed

View reviewed changes

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/JsonRpcHttpServer.scala Show resolved Hide resolved

biandratti reviewed

View reviewed changes

src/main/scala/io/iohk/ethereum/jsonrpc/server/http/RateLimit.scala

+                    extractClientIP { ip =>
+                      if (isBelowRateLimit(ip)) {
+                        val err = JsonRpcError.RateLimitError(minInterval)
+                        complete((StatusCodes.TooManyRequests, err))

Contributor

biandratti Dec 30, 2020

Just a minor comment, we can reduce this line:
complete((StatusCodes.TooManyRequests, JsonRpcError.RateLimitError(minInterval)))

Contributor Author

dmitry-worker Jan 1, 2021

Actually it was inline in the beginning, but I've decided that the code is quite unreadable :)
IMHO this will not affect performance and the compiler will inline it anyway because it is used once, immediately after creation. Maybe it would be better to construct a kind of builder for responses instead of serializable Tuples. WDYT?

biandratti approved these changes

View reviewed changes

Contributor

biandratti commented Dec 30, 2020

Just a minor comment, Maybe you can create the task mentioned. So then we can continue working with improvements.
Good job!


          Merge branch 'develop' into feature/ETCM-266-no-twitter-util-collection

c645eed

dmitry-worker merged commit 84a2f9b into develop

dzajkowski deleted the feature/ETCM-266-no-twitter-util-collection branch

April 9, 2021 12:02

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet