Use sorted set to limit with moving window by promag · Pull Request #33 · tj/node-ratelimiter

promag · 2017-02-07T15:42:15Z

No description provided.

noamshemesh · 2017-02-09T13:05:35Z

I don't want to embed cpp here, is there any other option? BTW the CI failed for node 4 and 5 (I will change to 6 soon)

promag · 2017-02-10T12:27:11Z

I've pushed a commit to use process.hrtime() instead of microtime. The idea is to low the chance of same score elements, ie, zadd has the highest chance to succeed.

EDIT: if this goes forward then I'll squash.

promag · 2017-02-10T16:47:22Z

index.js

-  function mget() {
-    db.watch([count], function (err) {
+  var now = Date.now() * 1000 + Math.floor(process.hrtime()[1] / 1000) % 1000;
+  var start = now - duration * 1000;


Should be (duration + 1) to take into account jitter from hrtime microseconds.

That's because hrtime is not stable enough for us :-\

noamshemesh

In general, I want to understand if we really need this. These operations are slower than only 3 straightforward sets, and I'm not sure what is the value.

Can you tell me more about the use case for this library that requires microseconds resolution?

noamshemesh · 2017-02-11T09:03:12Z

index.js

-
-  function mget() {
-    db.watch([count], function (err) {
+  var now = Date.now() * 1000 + Math.floor(process.hrtime()[1] / 1000) % 1000;


I'm not sure using process.hrtime is the best approach either. By definition it related to arbitrary time in the past.

noamshemesh · 2017-02-11T09:03:42Z

index.js

-  function mget() {
-    db.watch([count], function (err) {
+  var now = Date.now() * 1000 + Math.floor(process.hrtime()[1] / 1000) % 1000;
+  var start = now - duration * 1000;


That's because hrtime is not stable enough for us :-\

promag · 2017-02-20T22:55:05Z

@noamshemesh sorry for the delay.

Current implementation problems:

MULTI is not rollback'ed if the WATCH'ed key expires;
Each key (count, limit and reset) have different expire timestamps and MGET can return some keys expired;
MULTI is not rollback'ed if SET NX fails, so one key can be set while the others remain the same;

Given the above, and following the code, it can enter a loop create -> mget -> create -> mget -> ..., which results in a huge performance problem. This has been observed.

The way I see, the current implementation lacks for atomicity and it is unstable in a high concurrent system.

BTW, as stated here, tests fail randomly because of the above reasons.

The proposed solution doesn't suffer the above problems. It is only a bit more redis demanding
but much more concurrent friendly.

promag · 2017-02-20T22:58:43Z

@noamshemesh forgot to explain the need for microtime. ZADD key score value will add value to the sorted set and score must be unique. With score as milliseconds, even running the tests
causes ZADD to fail.

BTW, pushed a commit with an alternative solution regarding the microtime.

promag · 2017-02-23T11:42:01Z

Ping @noamshemesh.

noamshemesh · 2017-02-23T11:48:20Z

Sorry, crazy days, will take a look on the next weekend

…

On Thu, Feb 23, 2017, 13:42 João Barbosa ***@***.***> wrote: Ping @noamshemesh <https://github.com/noamshemesh>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE4fwYY2PtWjqhEctiU2fzgmFe6cY5iJks5rfXCKgaJpZM4L5p0o> .

noamshemesh · 2017-02-25T11:59:23Z

Looks good, thanks.
Please resolve the conflicts (it seems like you already fixed the previous PR, so just make sure you can ignore them), squash and I'll merge.

promag · 2017-02-26T11:33:47Z

@noamshemesh done.

BTW, if it would be awesome if you could create a release.

noamshemesh · 2017-02-26T12:41:46Z

Sure, I think I'll release a major version. what do you think?

…

On Sun, Feb 26, 2017, 13:33 João Barbosa ***@***.***> wrote: @noamshemesh <https://github.com/noamshemesh> done. BTW, if it would be awesome if you could create a release. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE4fwa0gSNr6Cdx5KejAtYOAUUclttyOks5rgWMcgaJpZM4L5p0o> .

promag · 2017-02-26T12:47:18Z

LGTM

noamshemesh · 2017-02-27T13:07:55Z

Done. Thanks a lot!

This was referenced Feb 7, 2017

Use hash instead of 3 keys #32

Closed

Handle expired limit and reset #31

Closed

promag force-pushed the enhancement/use-sorted-set branch from 7ca5a69 to 1d20bed Compare February 10, 2017 15:47

promag commented Feb 10, 2017

View reviewed changes

noamshemesh reviewed Feb 11, 2017

View reviewed changes

promag force-pushed the enhancement/use-sorted-set branch from 1d20bed to e8a76da Compare February 20, 2017 22:08

promag force-pushed the enhancement/use-sorted-set branch from e8a76da to 2f32b27 Compare February 20, 2017 23:01

promag force-pushed the enhancement/use-sorted-set branch from 2f32b27 to 7d124ed Compare February 26, 2017 11:19

Use sorted set to limit with moving window

2a56a04

promag force-pushed the enhancement/use-sorted-set branch from 7d124ed to 2a56a04 Compare February 26, 2017 11:23

noamshemesh added a commit that referenced this pull request Feb 27, 2017

Use sorted set to limit with moving window (PR #33 - @promag)

bf195e6

noamshemesh added a commit that referenced this pull request Feb 27, 2017

Use sorted set to limit with moving window (PR #33 - @promag)

782f0ff

noamshemesh merged commit 2a56a04 into tj:master Feb 27, 2017

noamshemesh added a commit that referenced this pull request Feb 27, 2017

Use sorted set to limit with moving window (PR #33 - @promag)

21b3354

Conversation

promag commented Feb 7, 2017

Uh oh!

noamshemesh commented Feb 9, 2017

Uh oh!

promag commented Feb 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag Feb 10, 2017

Choose a reason for hiding this comment

Uh oh!

noamshemesh Feb 11, 2017

Choose a reason for hiding this comment

Uh oh!

noamshemesh left a comment

Choose a reason for hiding this comment

Uh oh!

noamshemesh Feb 11, 2017

Choose a reason for hiding this comment

Uh oh!

noamshemesh Feb 11, 2017

Choose a reason for hiding this comment

Uh oh!

promag commented Feb 20, 2017

Uh oh!

promag commented Feb 20, 2017

Uh oh!

promag commented Feb 23, 2017

Uh oh!

noamshemesh commented Feb 23, 2017 via email

Uh oh!

noamshemesh commented Feb 25, 2017

Uh oh!

promag commented Feb 26, 2017

Uh oh!

noamshemesh commented Feb 26, 2017 via email

Uh oh!

promag commented Feb 26, 2017

Uh oh!

noamshemesh commented Feb 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

promag commented Feb 10, 2017 •

edited

Loading