Add workqueue support #624

cizezsy · 2019-07-15T11:09:20Z

Add workqueue support, see #523. Feel free to close this PR, if you think #525 is better :P

Will add more unit tests later.

yue9944882 · 2019-07-15T13:05:35Z

he's getting back on wednesday, let's hold until some input from him. by skimming the sketch, i find the rate-limiter enhancement compelling. do you consider port the leaky-bucket-based rate-limiter to the tree?

k8s-ci-robot · 2019-07-15T13:05:36Z

@yue9944882: GitHub didn't allow me to request PR reviews from the following users: adohe.

Note that only kubernetes-client members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @adohe

he's getting back on wednesday, let's hold until some input from him. by skimming the sketch, i find the rate-limiter enhancement compelling. do you consider port the leaky-bucket-based rate-limiter to the tree?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cizezsy · 2019-07-16T07:49:01Z

@yue9944882 Added it :P I will add more unit tests.

Also, the BucketRateLimiter is based on token bucket according to the documentation of rate.Limiter

It implements a "token bucket" of size b, initially full and refilled at rate r tokens per second.

extended/src/main/java/io/kubernetes/client/extended/workqueue/DelayingWorkQueue.java

extended/src/main/java/io/kubernetes/client/extended/workqueue/WorkQueue.java

extended/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/RateLimiter.java

brendandburns · 2019-07-16T17:20:51Z

I put in some suggestions about existing libraries/classes you could use instead of re-implementing things...

yue9944882

in favor of the rate-limiter related codes in this pull, how do you think about a rebase onto #525 and replace its rate-limiter impl (after the pull gets in)? the work-queue from the another pull is proven working as expected for a period of time in our internal system from our experience..

yue9944882 · 2019-07-17T11:52:12Z

you could use instead of re-implementing things...

+1 on we should avoid re-implementing wheels as much as possible

yue9944882 · 2019-07-19T03:57:03Z

@cizezsy another work-queue has got in, how do you think about rebase and replace the rate-limiter onto that?

cizezsy · 2019-07-19T05:25:57Z

@yue9944882 ok, works for me :P

cizezsy · 2019-07-22T11:05:57Z

@yue9944882 rebased and added some unit test :)

...a/io/kubernetes/client/extended/workqueue/ratelimiter/ItemExponentialFailureRateLimiter.java

...ded/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/BucketRateLimiter.java

yue9944882

thanks, overall LGTM. i'm trying simplifying a bit fractions. and we need some more tests to verify the behavior of these rate-limiters on unexpected parameters.

...ded/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/BucketRateLimiter.java

...a/io/kubernetes/client/extended/workqueue/ratelimiter/ItemExponentialFailureRateLimiter.java

...c/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/ItemFastSlowRateLimiter.java

…lureRateLimiter, add more unit tests

cizezsy · 2019-07-29T08:10:50Z

Sorry for the delay, I am too busy in last week....

I have changed the implementation of BucketRateLimiter and ItemExponentialFailureRateLimiter as comment, and added some unit tests.

yue9944882 · 2019-07-31T03:42:26Z

LGTM, @brendandburns may want another round of review?

brendandburns · 2019-07-31T20:26:14Z

...ded/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/BucketRateLimiter.java

+    @Override
+    public ScheduledFuture<?> schedule(Runnable command, long delay, TimeUnit unit) {
+      this.delay = Duration.ofNanos(unit.toNanos(delay));
+      return null;


Should this really return null?

@brendanburns

The return value will not be used by Bucket4j according to the comment of bucket4j/bucket4j#96, so I think it ok to return a null value.

ScheduledFuture that returned also not used by Bucket4j, so do not spend efforts to provide correctly result.

brendandburns · 2019-07-31T20:29:32Z

A few additional comments.

...ded/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/BucketRateLimiter.java

yue9944882 · 2019-08-02T04:32:43Z

/lgtm
/approve
/hold

holding for lazy consensus with @brendandburns

k8s-ci-robot · 2019-08-02T04:32:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cizezsy, yue9944882

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yue9944882]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

brendandburns · 2019-08-02T21:02:53Z

/hold remove

brendandburns · 2019-08-02T21:03:24Z

/hold cancel

k8s-ci-robot requested review from mbohlool and yue9944882 July 15, 2019 11:09

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 15, 2019

brendandburns reviewed Jul 16, 2019

View reviewed changes

extended/src/main/java/io/kubernetes/client/extended/workqueue/DelayingWorkQueue.java Outdated Show resolved Hide resolved

brendandburns reviewed Jul 16, 2019

View reviewed changes

extended/src/main/java/io/kubernetes/client/extended/workqueue/WorkQueue.java Outdated Show resolved Hide resolved

brendandburns reviewed Jul 16, 2019

View reviewed changes

extended/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/RateLimiter.java Show resolved Hide resolved

yue9944882 reviewed Jul 17, 2019

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2019

Feat: add rate limiters

9f73d82

cizezsy force-pushed the feat/workqueue branch from a2d18d1 to 9f73d82 Compare July 22, 2019 10:54

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2019

yue9944882 reviewed Jul 23, 2019

View reviewed changes

...a/io/kubernetes/client/extended/workqueue/ratelimiter/ItemExponentialFailureRateLimiter.java Outdated Show resolved Hide resolved

yue9944882 reviewed Jul 23, 2019

View reviewed changes

...ded/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/BucketRateLimiter.java Show resolved Hide resolved

yue9944882 reviewed Jul 23, 2019

View reviewed changes

adohe-zz reviewed Jul 23, 2019

View reviewed changes

...ded/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/BucketRateLimiter.java Outdated Show resolved Hide resolved

brendandburns reviewed Jul 23, 2019

View reviewed changes

...a/io/kubernetes/client/extended/workqueue/ratelimiter/ItemExponentialFailureRateLimiter.java Outdated Show resolved Hide resolved

adohe-zz reviewed Jul 24, 2019

View reviewed changes

...c/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/ItemFastSlowRateLimiter.java Outdated Show resolved Hide resolved

Change the implementation of BucketRateLimiter and ItemExponentialFai…

e85e988

…lureRateLimiter, add more unit tests

brendandburns reviewed Jul 31, 2019

View reviewed changes

yue9944882 reviewed Aug 1, 2019

View reviewed changes

...ded/src/main/java/io/kubernetes/client/extended/workqueue/ratelimiter/BucketRateLimiter.java Outdated Show resolved Hide resolved

cizezsy force-pushed the feat/workqueue branch from 862611e to f5f5a9c Compare August 1, 2019 06:22

Use AtomicLongMap to calculate attempts

4afdc8d

cizezsy force-pushed the feat/workqueue branch from f5f5a9c to 4afdc8d Compare August 1, 2019 06:34

k8s-ci-robot assigned yue9944882 Aug 2, 2019

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 2, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 2, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 2, 2019

k8s-ci-robot merged commit 89b802e into kubernetes-client:master Aug 2, 2019

Add workqueue support #624

Add workqueue support #624

Uh oh!

Conversation

cizezsy commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yue9944882 commented Jul 15, 2019

Uh oh!

k8s-ci-robot commented Jul 15, 2019

Uh oh!

cizezsy commented Jul 16, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brendandburns commented Jul 16, 2019

Uh oh!

yue9944882 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yue9944882 commented Jul 17, 2019

Uh oh!

yue9944882 commented Jul 19, 2019

Uh oh!

cizezsy commented Jul 19, 2019

Uh oh!

cizezsy commented Jul 22, 2019

Uh oh!

Uh oh!

Uh oh!

yue9944882 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cizezsy commented Jul 29, 2019

Uh oh!

yue9944882 commented Jul 31, 2019

Uh oh!

brendandburns Jul 31, 2019

Choose a reason for hiding this comment

Uh oh!

cizezsy Aug 1, 2019

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Jul 31, 2019

Uh oh!

Uh oh!

yue9944882 commented Aug 2, 2019

Uh oh!

k8s-ci-robot commented Aug 2, 2019

Uh oh!

brendandburns commented Aug 2, 2019

Uh oh!

brendandburns commented Aug 2, 2019

Uh oh!

Uh oh!

cizezsy commented Jul 15, 2019 •

edited

Loading

yue9944882 left a comment •

edited

Loading