add workqueue implementation for kube-style controller #525

adohe-zz · 2019-03-09T08:01:53Z

Need more tests.

brendandburns · 2019-03-09T22:02:57Z

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

+import java.util.concurrent.TimeUnit;
+
+/**
+ * The default delaying queue implementation.


Please add more documentation about what this is and how it is used. We can't assume people are familiar with the client go approach.

brendandburns · 2019-03-09T22:03:29Z

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

+    private DelayQueue      delayQueue    = new DelayQueue();
+    private ExecutorService waitingWorker = Executors.newSingleThreadExecutor();
+
+    public DefaultDelayingQueue() {


It's probably worth making the ExecutorService an optional parameter, for testing and other reasons.

brendandburns · 2019-03-09T22:04:40Z

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

+/**
+ * The default delaying queue implementation.
+ */
+public class DefaultDelayingQueue extends DefaultWorkQueue implements DelayingQueue {


Use generic types instead of just passing Object

brendandburns · 2019-03-09T22:05:22Z

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

+        }
+    }
+
+    private class WaitForEntry implements Delayed {


make this class static. (alternately, just use an anonymous class above)

I think I prefer the anonymous class approach.

just use static inner class.

brendandburns · 2019-03-09T22:05:57Z

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

+/**
+ * The default rate limiting queue implementation.
+ */
+public class DefaultRateLimitingQueue extends DefaultDelayingQueue implements RateLimitingQueue {


As above wrt generics.

brendandburns · 2019-03-09T22:11:01Z

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

+    private final Condition notEmpty = lock.newCondition();
+
+    public DefaultWorkQueue() {
+        this.queue = new LinkedList<>();


Lists.newLinkedList() (or don't use Sets below...)

brendandburns · 2019-03-09T22:12:04Z

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

+
+    private AtomicBoolean shuttingDown = new AtomicBoolean(false);
+
+    private final ReentrantLock lock = new ReentrantLock();


Get rid of this lock and just used synchronized on the methods.

brendandburns · 2019-03-09T22:12:35Z

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

+        lock.lock();
+        try {
+            while (queue.size() == 0 && !shuttingDown.get()) {
+                notEmpty.await();


use this.wait() and this.notify() instead of condition variable.

brendandburns · 2019-03-09T22:14:57Z

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

+     * Get blocks until it can return an item to be processed. If shutdown = true,
+     * the caller should end their process.
+     *
+     * @return the object


which object?

brendandburns · 2019-03-09T22:15:44Z

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

+/**
+ * The workqueue interface defines the queue behavior.
+ */
+public interface WorkQueue {


Make this a generic instead of using Object

brendandburns · 2019-03-09T22:16:51Z

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

+
+import java.time.Duration;
+
+public interface RateLimiter {


There are lots of other retry libraries out there (e.g. https://github.com/jhalterman/failsafe) do we need to implement this ourselves?

didn't notice this before, will take a deep look

brendandburns · 2019-03-09T22:18:12Z

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

+            this.processing.add(obj);
+            this.dirty.remove(obj);
+            return obj;
+        } catch (Throwable t) {


Why do you want to catch here? this seems like a bad idea... (at the very least we should log)

this depends on how to handle exception, throw exception out here seems better

brendandburns · 2019-03-09T22:19:31Z

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

+
+    @Override
+    public void shutDown() {
+        this.shuttingDown.compareAndSet(false, true);


I think you need to notify() here too...

sure, should use notifyAll() here

brendandburns · 2019-03-09T22:22:13Z

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

+import java.util.concurrent.locks.ReentrantLock;
+
+/**
+ * The default workqueue implementation.


You should document how you expect this to be used.

brendandburns · 2019-03-09T22:23:54Z

I have three high level comments:

Please use generics everywhere.
Please add much better documentation about usage, I don't think anyone reading this library will know how to use it.
If we only ever expect to have one implementation, let's not have interfaces, let's just have the implementation.

Other comments: More tests are definitely required, some sample code would be great.

yue9944882 · 2019-03-10T06:29:00Z

thx so much for help w/ this @adohe !

do you think about splitting this pull into two parts? one for initializing the new module, another for the implementation so that it won't be the blocker for other tasks. thx!

Please use generics everywhere.

+1

yue9944882 · 2019-04-30T16:52:33Z

@adohe seems recently too swamped to bump the thread. let me take over it. we'll finally get a java controller example into this repo.

adohe-zz · 2019-05-01T07:46:35Z

comments addressed, I am working on much better usage documentation and test cases. cc @brendandburns @yue9944882

yue9944882 · 2019-05-13T13:19:20Z

ci looks not happy

cizezsy · 2019-06-22T03:58:09Z

Any update on this PR? I really want to use this feature in client-java :)

yue9944882 · 2019-06-26T17:47:04Z

@cizezsy the guy is recently too busy to push the progress. and also, the pull is a bit more complex so it might take time. welcome to split an another pass if you want to contribute.

yue9944882 · 2019-07-10T12:57:19Z

extended/src/main/java/io/kubernetes/client/extended/function/ConditionFunc.java

+package io.kubernetes.client.extended.function;
+
+@FunctionalInterface
+public interface ConditionFunc {


remove it, use functional.Supplier as replacement

yue9944882 · 2019-07-17T11:31:07Z

/lgtm
/approve
/hold

lazy consensus if @brendandburns would like another rounds of review

k8s-ci-robot · 2019-07-17T11:31:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adohe, 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

yue9944882 · 2019-07-19T03:35:51Z

/hold cancel

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 9, 2019

k8s-ci-robot requested review from brendandburns and mbohlool March 9, 2019 08:01

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 9, 2019

yue9944882 mentioned this pull request Mar 9, 2019

[Umbrella Issue] Java SDK utility tooling improvement plan #523

Closed

10 tasks

brendandburns reviewed Mar 9, 2019

View reviewed changes

adohe-zz force-pushed the master branch 2 times, most recently from c2563f2 to 5fc35cd Compare May 1, 2019 07:39

adohe-zz force-pushed the master branch 2 times, most recently from 18f92b2 to bd4ada9 Compare May 29, 2019 06:30

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 29, 2019

adohe-zz force-pushed the master branch from bd4ada9 to 6053b7b Compare July 10, 2019 11:00

yue9944882 reviewed Jul 10, 2019

View reviewed changes

cizezsy mentioned this pull request Jul 15, 2019

Add workqueue support #624

Merged

adohe-zz force-pushed the master branch 3 times, most recently from a6c2951 to b862d68 Compare July 17, 2019 01:06

workqueue implementation for kube-style controller

9fcb7dd

adohe-zz force-pushed the master branch from b862d68 to 9fcb7dd Compare July 17, 2019 07:26

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

k8s-ci-robot assigned yue9944882 Jul 17, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 17, 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 Jul 19, 2019

k8s-ci-robot merged commit 40982b5 into kubernetes-client:master Jul 19, 2019


		private AtomicBoolean shuttingDown = new AtomicBoolean(false);

		private final ReentrantLock lock = new ReentrantLock();

add workqueue implementation for kube-style controller #525

add workqueue implementation for kube-style controller #525

Uh oh!

Conversation

adohe-zz commented Mar 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendandburns commented Mar 9, 2019

Uh oh!

yue9944882 commented Mar 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yue9944882 commented Apr 30, 2019

Uh oh!

adohe-zz commented May 1, 2019

Uh oh!

yue9944882 commented May 13, 2019

Uh oh!

cizezsy commented Jun 22, 2019

Uh oh!

yue9944882 commented Jun 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yue9944882 commented Jul 17, 2019

Uh oh!

k8s-ci-robot commented Jul 17, 2019

Uh oh!

yue9944882 commented Jul 19, 2019

Uh oh!

Uh oh!

adohe-zz commented Mar 9, 2019 •

edited

Loading

yue9944882 commented Mar 10, 2019 •

edited

Loading