Skip to content
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

Use sorted set to limit with moving window #33

Merged
merged 1 commit into from
Feb 27, 2017
Merged

Use sorted set to limit with moving window #33

merged 1 commit into from
Feb 27, 2017

Conversation

promag
Copy link
Contributor

@promag promag commented Feb 7, 2017

No description provided.

This was referenced Feb 7, 2017
Closed
Closed
@noamshemesh
Copy link
Collaborator

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
Copy link
Contributor Author

promag commented Feb 10, 2017 •
edited
Loading

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
promag commented Feb 10, 2017
View reviewed changes
index.js Outdated
function mget() {
db.watch([count], function (err) {
var now = Date.now() * 1000 + Math.floor(process.hrtime()[1] / 1000) % 1000;
var start = now - duration * 1000;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

noamshemesh
noamshemesh reviewed Feb 11, 2017
View reviewed changes
Copy link
Collaborator

@noamshemesh noamshemesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

index.js Outdated

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@promag
Copy link
Contributor Author

promag commented Feb 20, 2017

@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
Copy link
Contributor Author

promag commented Feb 20, 2017

@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
Copy link
Contributor Author

promag commented Feb 23, 2017

Ping @noamshemesh.

@noamshemesh
Copy link
Collaborator

noamshemesh commented Feb 23, 2017 via email

@noamshemesh
Copy link
Collaborator

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
Copy link
Contributor Author

promag commented Feb 26, 2017

@noamshemesh done.

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

@noamshemesh
Copy link
Collaborator

noamshemesh commented Feb 26, 2017 via email

@promag
Copy link
Contributor Author

promag commented Feb 26, 2017

LGTM

noamshemesh added a commit that referenced this pull request Feb 27, 2017
@noamshemesh
Use sorted set to limit with moving window (PR #33 - @promag)
bf195e6
noamshemesh added a commit that referenced this pull request Feb 27, 2017
@noamshemesh
Use sorted set to limit with moving window (PR #33 - @promag)
782f0ff
@noamshemesh noamshemesh merged commit 2a56a04 into tj:master Feb 27, 2017
noamshemesh added a commit that referenced this pull request Feb 27, 2017
@noamshemesh
Use sorted set to limit with moving window (PR #33 - @promag)
21b3354
@noamshemesh
Copy link
Collaborator

Done. Thanks a lot!

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

@noamshemesh noamshemesh noamshemesh left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@promag @noamshemesh