Replicate counter identity rules

[mdimjasevic]

This patch closes #13. It introduces a time unit to CounterKey to differentiate rules that are the same in everything but the time unit. If the time unit stays the same, but the rate limit changes, fencer does the same as lyft/ratelimit.

[effectfully]

LGTM.

[neongreen]

How is this different from fromJust?

[effectfully]

It prints the stack trace when something goes wrong, so that it's at least clear where the error happened. fromJust outputs

*** Exception: Maybe.fromJust: Nothing

and no indication whatsoever where things went wrong.

[neongreen]

I have several suggestions regarding this test.

First of all, it does not test the actual logic used by the server. It does the lookup on its own instead of using getLimit. To make it more faithful, I would propose:

• moving shouldRateLimitDescriptor to Fencer.AppState and renaming it
• perhaps also renaming Fencer.AppState to Fencer.Logic or something
• removing the export of recordHits and not using recordHits in the test

Then the test would also get moved to Fencer.Logic.Test since it's not related to the Fencer.Rules module.

Secondly, here's what this test does:

  load definitions1
  hit counterKey1

  load definitions2
  assert (counterKey1 hasn't changed after 'load definitions2')
  assert (counterKey2 doesn't exist after 'load definitions2')

This does not match the description of the test ("Test that a rule limit unit change adds a new counter [...]"). Also, there are bad implementations it does not catch – for instance, what if counters are removed during recordHits and not during loadRules?

Here's a scenario I would propose instead:

  // change the limit to e.g. 4 instead of 400000

  load definitions1
  status <- hit "generic_key/dream11_order_create"
  assert (counterRemainingLimit status == 3)

  load definitions2
  status <- hit "generic_key/dream11_order_create"
  assert (counterRemainingLimit status == 3)  // unaffected by the previous 'hit'

  load definitions1
  status <- hit "generic_key/dream11_order_create"
  assert (counterRemainingLimit status == 2)  // the old counter has persisted!

[neongreen]

Is this the reason why you got rid of tasty-discover? If yes, perhaps let's instead disable the parallelism by running tests with +RTS -N1 -RTS.

[mdimjasevic]

Right, I couldn't set the order of test execution when tasty-discover was used.

If I am not missing anything, +RTS -N1 -RTS would mean the program hangs because it would not be able to fork a thread and start a gRPC server.

[effectfully]

@neongreen

If yes, perhaps let's instead disable the parallelism by running tests with +RTS -N1 -RTS.

I don't think it's a good idea as it solves a pretty much already solved problem, but also makes tests slower.

@mdimjasevic

If I am not missing anything, +RTS -N1 -RTS would mean the program hangs because it would not be able to fork a thread and start a gRPC server.

Threads are forkable with -threaded even with a single core.

[neongreen]

There should be a comment about this behavior, then.

…

On 21. Oct 2019, 18:08 +0300, effectfully ***@***.***>, wrote: @effectfully commented on this pull request. In test/Fencer/Rules/Test.hs: > + TIO.writeFile (tempDir </> dir </> cfgFile) txt + loadRulesFromDirectory (#directory tempDir) (#ignoreDotFiles True) + + ruleKey = RuleKey "generic_key" + ruleValue = RuleValue "dream11_order_create" + domainId = DomainId "merchant_rate_limits" + + counterKey1 = CounterKey + { counterKeyDomain = domainId + , counterKeyDescriptor = [ (ruleKey, ruleValue) ] + , counterKeyUnit = Minute } + + counterKey2 :: CounterKey + counterKey2 = counterKey1 { counterKeyUnit = Hour } + + fromMaybe' :: Maybe a -> a It prints the stack trace when something goes wrong, so that it's at least clear where the error happened. fromJust outputs *** Exception: Maybe.fromJust: Nothing and no indication whatsoever where things went wrong. — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.