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.

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

1070 Add discard config for rate-limiter #1096

Merged
merged 2 commits into from Oct 24, 2018

Conversation

Projects
None yet
3 participants
@aleccool213
Copy link
Contributor

aleccool213 commented Oct 22, 2018

  • Meant for Manually Fetching Jobs pattern

  • closes #1070

1070 Add discard config for rate-limiter
- Meant for Manually Fetching Jobs pattern
});

it('should put a job into the delayed queue when limit is hit', function() {
queue.on('failed', function(e) {

This comment has been minimized.

@csingh

csingh Oct 23, 2018

Might be better to create a new queue here as you did in the test below, since this test assumes that the queue max is set to 1.

})
).then(function() {
return newQueue.getDelayedCount().then(function(delayedCount) {
expect(delayedCount).to.eq(0);

This comment has been minimized.

@csingh

csingh Oct 23, 2018

Should also do an assert on the main queue to make sure 3 objects are there. Might also want to assert the ordering of the jobs to make sure that is untouched?

jobCounter = tonumber(rcall("GET", KEYS[6]))
if jobCounter ~= nil and jobCounter >= maxJobs then
if discard == 'true' then

This comment has been minimized.

@csingh

csingh Oct 23, 2018

Are there any tests for these .lua scripts? If so, we should update them for this case.

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

there are not test only for the .lua scripts, but integration tests that indirectly call this .lua scripts.

PATTERNS.md Outdated

Patterns
========
# Patterns

This comment has been minimized.

@csingh

csingh Oct 23, 2018

Do we need all these formatting changes in this PR? :) I would remove if unnecessary, its cluttering things up and distracting from the feature-related changes

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

there are great changes though, would be nice to have them in a different PR, but if to cumbersome we could accept it in this one.

This comment has been minimized.

@aleccool213

aleccool213 Oct 24, 2018

Author Contributor

will make another pr

This comment has been minimized.

@aleccool213

aleccool213 Oct 24, 2018

Author Contributor

https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint was used and automatically formatted it for me on save

duration: number, // per duration in milliseconds
max: number; // Max number of jobs processed
duration: number; // per duration in milliseconds
discard: boolean = false; // When jobs get rate limited, nothing happens

This comment has been minimized.

@csingh

csingh Oct 23, 2018

discard might not be the best name for this.. because we're not entirely discarding the rate limiter, we are just disabling the delayed queue. So maybe disableDelyedQueue or something similar? And then change the comment to say something more explicit like When set to true and jobs get rate limited, they stay in the main queue and are not moved to the delayed queue

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

ok, now I full understand the use-case and discard may indeed not be the best name. I thought the idea was to really throw away the jobs that get "throttled". so this name will be misleading. On the other hand, a name like disableDelayedQeuue is a bit techie and exposes implementation details.

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

Not sure about this name but what about this alternative bounceBack ? the option is quite special since it only has use in the manual processing pattern...

This comment has been minimized.

@aleccool213

aleccool213 Oct 24, 2018

Author Contributor

I like bounceBack 👍

PATTERNS.md Outdated

Patterns
========
# Patterns

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

there are great changes though, would be nice to have them in a different PR, but if to cumbersome we could accept it in this one.

jobCounter = tonumber(rcall("GET", KEYS[6]))
if jobCounter ~= nil and jobCounter >= maxJobs then
if discard == 'true' then

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

there are not test only for the .lua scripts, but integration tests that indirectly call this .lua scripts.

assert.fail(e);
});

return Promise.all(

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

maybe it is just better to write this just as:
Promise.all([queue.add({}), queue.add({}), queue.add({}), queue.add({})])

This comment has been minimized.

@aleccool213

aleccool213 Oct 24, 2018

Author Contributor

👍

duration: number, // per duration in milliseconds
max: number; // Max number of jobs processed
duration: number; // per duration in milliseconds
discard: boolean = false; // When jobs get rate limited, nothing happens

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

ok, now I full understand the use-case and discard may indeed not be the best name. I thought the idea was to really throw away the jobs that get "throttled". so this name will be misleading. On the other hand, a name like disableDelayedQeuue is a bit techie and exposes implementation details.

duration: number, // per duration in milliseconds
max: number; // Max number of jobs processed
duration: number; // per duration in milliseconds
discard: boolean = false; // When jobs get rate limited, nothing happens

This comment has been minimized.

@manast

manast Oct 23, 2018

Member

Not sure about this name but what about this alternative bounceBack ? the option is quite special since it only has use in the manual processing pattern...

@aleccool213

This comment has been minimized.

Copy link
Contributor Author

aleccool213 commented Oct 24, 2018

@manast new review is up 👍

@manast manast merged commit 118dba7 into OptimalBits:master Oct 24, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls First build on 1070 at 93.829%
Details

@aleccool213 aleccool213 deleted the wishabi:1070 branch Oct 26, 2018

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