[JENKINS-50568] make receive queue max threads configurable #79

mawinter69 · 2018-04-06T14:09:02Z

use a ThreadPoolExecutor to process the received events. This allows to
easily reconfigure the number of threads similar to the GerritSendQueue

use a ThreadPoolExecutor to process the received events. This allows to easily reconfigure the number of threads.

mawinter69 · 2018-04-06T14:40:50Z

For the Jenkins gerrit-trigger plugin, a change is also required to provide a ThreadPoolFactory that is jenkins aware (runs with Jenkins system ACL)

rsandell

Sorry for not responding until now, I've been quite busy on other things.

rsandell · 2018-04-20T13:26:25Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritDefaultValues.java

@@ -84,6 +84,10 @@ private GerritDefaultValues() {
     * The default nr of event worker threads.
     */
    public static final int DEFAULT_NR_OF_RECEIVING_WORKER_THREADS = 3;
+    /**
+     * The default nr of event worker threads.


Copy paste error?

rsandell · 2018-04-20T13:43:32Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

        this.numberOfWorkerThreads = numberOfWorkerThreads;
+        this.threadKeepAliveTime = threadKeepAliveTime;


Since executor.allowCoreThreadTimeOut(true); is specified, I think we need to specify some minimum safe number here. According to the documentation setting for example the timeout to 0 will cause new threads to be created all the time and thrown away upon completion. And I think that might not be desirable.

what would be a reasonable minimum value? 30 seconds? Maybe we could also use a fixed value (not allowing it to be changed but that makes testing harder and when you reduce the number of threads it might take quite long until they die.

I think we can go lower, even 10 seconds. IIUC as long as it's not 0 the core thread count should stay the same.

rsandell · 2018-04-20T13:44:53Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

+     * Returns the largest number of threads that have ever simultaneously been in the pool.
+     * @return number of threads
+     */
+    int getLargestPoolSize() {


why package private?

Does it have any value for users to know what is the max poolsize that was encountered? I need this method for testing to be at least package private.

ok, in those cases a comment explaining it's there for testability is common :)

rsandell · 2018-04-20T13:49:13Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

+        pool.shutdown(); // Disable new tasks from being submitted
+        try {
+            // Wait a while for existing tasks to terminate
+            if (!pool.awaitTermination(WAIT_FOR_JOBS_SHUTDOWN_TIMEOUT, TimeUnit.SECONDS)) {


You should only awaitTermination if join == true

rsandell · 2018-04-20T13:51:04Z

src/test/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandlerQueueTest.java

+        handler.addListener(listener);
+        postEventsToQueue(5);
+        waitForEventsProcessed();
+        assertThat(handler.getLargestPoolSize(), equalTo(3));


Better to use the constant here, so an onlooker understands it tests the default number

rsandell · 2018-04-20T13:53:59Z

src/test/java/com/sonymobile/tools/gerrit/gerritevents/SSHUtilTest.java

-        URL url = Thread.currentThread().getContextClassLoader().getResource(
-                "com/sonymobile/tools/gerrit/gerritevents/id_rsa");
-        File file = new File(url.getPath());
+        File file = new File("src/test/resources/com/sonymobile/tools/gerrit/gerritevents/id_rsa");


Why did you change this?
this won't work when running the tests from the published test jar

ok, but with the url it doesn't work with maven (at least on windows)

Yea, I would guess this is full of test code not working on windows :) Though still a bit strange why this wouldn't work.

rsandell · 2018-04-20T13:54:45Z

src/test/java/com/sonymobile/tools/gerrit/gerritevents/workers/EventThreadTest.java

@@ -77,6 +78,7 @@ public static void beforeClass() {
     * @throws Exception if so.
     */
    @Test(timeout = 1000)
+    @Ignore


Probably better to just remove these tests

rsandell · 2018-04-20T13:55:42Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

@@ -183,6 +276,7 @@ private void post(Work work) {
        try {
            logger.trace("putting work on queue.");
            workQueue.put(work);
+            queueWork(work);


Why double storage of the events? You seem to have removed most of the workQueue usage already.

I will remove this completely, but then the getWorkQueue method should also be removed which is an API change.

executor.getQueue() ?

executor.getQueue() returns BlockingQueue<Runnable> while the interface defines BlockingQueue<Work>

You could then keep the workQueue reference to the queue you give the executor. They would then be the same instance but with the correct type. Or just cast it since we control what goes in it I guess, both ways are slightly ugly in their own ways :)

Ah I got a bit confused there thinking that Work was a the Runnable that got added to the queue, but you actually introduced a new class for that. So getWorkQueue needs to stay for binary compatibility reasons, but it would be deprecated and since a lot of the information about the internal queue is exposed via other methods something half drastic could be done to keep it functioning a bit as before. For example iterate over the internal queue and creating a new Queue every time containing the Work objects or something.

Maybe we can make Work implement also Runnable.
Then we could cast executor.getQueue() to BlockingQueue<Work>

Not very easy without breaking more compatibility stuff, the way it's designed with the 'perform(Coordinator)' method.
But the simple fix to keep it would be something like

List<Work> l = new ArrayList<>(); for (Runnable r : executor.getQueue()) { if (r instanceof EventWorker) { l.add(((EventWorker)l).work); } }

I think that would iterate normally without blocking, but needs testing.

There might be some callers that are expecting to get the reference to the actual queue, so the javadoc needs to be updated as well. But at least we keep binary compatibility and hopefully the deprecation warning will make it so no user uses it any more.

set minimum value for keep alive time remove EventThread class

rsandell · 2018-04-23T13:27:44Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

+     */
+    public GerritHandler(int numberOfWorkerThreads, int threadKeepAliveTime) {
+        this.numberOfWorkerThreads = numberOfWorkerThreads;
+        if (threadKeepAliveTime < MIN_RECEIVE_THREAD_KEEP_ALIVE_TIME)


rsandell · 2018-04-23T13:35:10Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/workers/Coordinator.java

-     * Retrieves the work queue for workers to poll.
-     * @return the queue
-     */
-    BlockingQueue<Work> getWorkQueue();


No, unfortunately we can't change binary signature like this. Something else needs to be done.

rsandell · 2018-04-23T13:36:16Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/workers/EventThread.java

- * The idea is to split up as much work as possible to be able to quickly handle the next event from Gerrit
- * @author Robert Sandell &lt;robert.sandell@sonyericsson.com&gt;
- */
-public class EventThread extends Thread {


Speaking of binary compatibility, this needs to stay but be deprecated.

update javadoc

rsandell · 2018-05-16T10:04:07Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

+     *
+     * @param threadKeepAliveTime number of seconds
+     */
+    public void setThreadKeepAliveTime(int threadKeepAliveTime) {


Needs same MIN_RECEIVE_THREAD_KEEP_ALIVE_TIME guard as in the constructor

also check for null

rsandell · 2018-05-16T10:08:31Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

-        return workQueue;
+
+        BlockingQueue<Work> queue = new LinkedBlockingQueue<Work>();
+        BlockingQueue<Runnable> workQueue = executor.getQueue();


check for null

rsandell · 2018-05-16T10:09:41Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

+     * @return number of threads
+     */
+    int getLargestPoolSize() {
+        return executor.getLargestPoolSize();


check for null

rsandell · 2018-05-16T10:11:18Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

-            worker.shutdown();
+        if (executor != null) {
+            ThreadPoolExecutor pool = executor;
+            executor = null;


there are places throughout in the code that are assuming a non null executor. See above comments but I have most likely not found them all.

actually executor can't be null. It is initialized in the constructor.

rsandell · 2018-05-16T10:16:04Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/workers/Coordinator.java

 * @author Robert Sandell &lt;robert.sandell@sonyericsson.com&gt;
 */
 public interface Coordinator {
    /**
     * Retrieves the work queue for workers to poll.
     * @return the queue
     */
+    @Deprecated


Needs a walkthrough of gerrit-trigger-plugin to figure out what it needed this for and probably add whatever it actually needs. A bit iffy in terms of binary compatibility. It might be time to go to java 8 and add default methods, or introduce a Coordinator2 interface?

Gerrit trigger plugin is not using the getWorkQueue() method.

rsandell · 2018-05-16T10:17:25Z

src/main/java/com/sonymobile/tools/gerrit/gerritevents/GerritHandler.java

     */
-    protected EventThread createEventThread(String threadName) {


oh, was this removed as well, sorry I missed it. Same binary compatibility problems as the other removed methods I think.

recover protected method for binary compatibility

[JENKINS-50568] make receive queue max threads configurable

4ce06a9

use a ThreadPoolExecutor to process the received events. This allows to easily reconfigure the number of threads.

rsandell requested changes Apr 20, 2018

View reviewed changes

mawinter69 added 2 commits April 20, 2018 16:55

remove workQueue completely

4f30c54

set minimum value for keep alive time remove EventThread class

explain why package visiblity

fefc6b4

rsandell reviewed Apr 23, 2018

View reviewed changes

rsandell requested changes Apr 23, 2018

View reviewed changes

recover methods for binary compatibility

ffd2b42

update javadoc

rsandell requested changes May 16, 2018

View reviewed changes

review feedback

f6f9a5b

recover protected method for binary compatibility

rsandell approved these changes Sep 11, 2018

View reviewed changes

rsandell merged commit 54730d0 into sonyxperiadev:master Sep 11, 2018

rsandell mentioned this pull request Nov 13, 2018

support triggering for wip-state-changed and private-state-changed jenkinsci/gerrit-trigger-plugin#383

Merged

		this.numberOfWorkerThreads = numberOfWorkerThreads;
		this.threadKeepAliveTime = threadKeepAliveTime;

		*/
		protected EventThread createEventThread(String threadName) {

[JENKINS-50568] make receive queue max threads configurable #79

[JENKINS-50568] make receive queue max threads configurable #79

Conversation

mawinter69 commented Apr 6, 2018

mawinter69 commented Apr 6, 2018

rsandell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mawinter69 Apr 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mawinter69 Apr 20, 2018 •

edited

Loading