[FIXED JENKINS-36144] Borrow the SCMTrigger's queue by stephenc · Pull Request #126 · jenkinsci/github-plugin

stephenc · 2016-06-22T11:21:06Z

https://issues.jenkins-ci.org/browse/JENKINS-36144

@reviewbybees @jenkinsci/code-reviewers

This change is

…its queue size)

ghost · 2016-06-22T11:23:16Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

KostyaSha · 2016-06-22T11:29:07Z

src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java

+                    maximumThreads = count;
+                    try {
+                        Field getQueue = SCMTrigger.DescriptorImpl.class.getDeclaredField("queue");
+                        getQueue.setAccessible(true);


of course... but after a more careful analysis I decided against that and rather just mirror the pool size... which sadly means you have two pools... but this does point that perhaps we should have an explicit pool for polling that plugins can use rather than the current status where each push notifier has to roll their own... with hillarious consequences

…er not to borrow - we will mirror the count though

jtnord · 2016-06-22T13:17:03Z

🐝 @stephenc I assume you are going to file a issue for the generic issue

but after a more careful analysis I decided against that and rather just mirror the pool size... which sadly means you have two pools... but this does point that perhaps we should have an explicit pool for polling that plugins can use rather than the current status where each push notifier has to roll their own... with hillarious consequences

rsandell · 2016-06-22T13:25:37Z

🐝 A future further improvement could perhaps also be to not schedule the polling if the same repo and branch is already in the queue.

KostyaSha · 2016-06-22T13:34:17Z

Queue skipping polling for the same project AFAIK

KostyaSha · 2016-06-22T13:35:13Z

A future further improvement could perhaps also be to not schedule the polling if the same repo and branch is already in the queue.

Could you describe how queue works now?

stephenc · 2016-06-22T14:07:20Z

https://issues.jenkins-ci.org/browse/JENKINS-36147 filed @jtnord

lanwen · 2016-06-22T19:23:42Z

src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java

            return ALLOW_HOOKURL_OVERRIDE;
        }

+        private static ThreadFactory threadFactory() {


it should be method that returns executors - threadPoolWithCapacity(int)
(it allows to reduce complexity of checkThreadPoolSize)

I don't see the value, seems like change for change's sake. The other method is not too complex whereas we'd need to add a three way test to use this as an executor service factory method

Don't think the complexity reduction in such simple way is "like change for change's sake". At minimal state it should be named as namedThreadFactory as of its simple shortcut.

jtnord · 2016-06-22T19:36:27Z

🐛 as I think the Executor service has the potential to leak threads (at least temporarily until some GC kicks in, the point of which is non-deterministic)

ghost · 2016-06-22T20:11:31Z

I think you need to look at queue.setExecutor

Sent from my iPhone

On 22 Jun 2016, at 20:36, James Nord notifications@github.com wrote:

🐛 as I think the Executor service has the potential to leak threads (at least temporarily until some GC kicks in, the point of which is non-deterministic)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

You received this message because you are subscribed to the Google Groups "engineering-code-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email to engineering-code-reviews+unsubscribe@cloudbees.com.
To post to this group, send email to engineering-code-reviews@cloudbees.com.
To view this discussion on the web visit https://groups.google.com/a/cloudbees.com/d/msgid/engineering-code-reviews/jenkinsci/github-plugin/pull/126/c227853208%40github.com.

jtnord · 2016-06-22T22:43:08Z

Yes, my bad. 🐝

dummy-lanwen-bot · 2016-06-22T23:14:52Z

src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java

+                if (maximumThreads != count) {
+                    maximumThreads = count;
+                    queue.setExecutors(
+                            (count == 0


Remove those useless parentheses.

dummy-lanwen-bot · 2016-06-22T23:14:54Z

SonarQube analysis reported 1 issue:

1 major

Watch the comments in this conversation to review them.

KostyaSha · 2016-06-23T20:09:08Z

Btw, the same queue i'm using in other trigger plugins. Is it possible to make queue public and place configuration in global config?

ghost · 2016-06-23T20:26:48Z

That is the RFE against core that I filed BTW. What I may do is add a proxy implementation into SCM-API so that people can use against earlier cores

Sent from my iPhone

On 23 Jun 2016, at 21:09, Kanstantsin Shautsou notifications@github.com wrote:

Btw, the same queue i'm using in other trigger plugins. Is it possible to make queue public and place configuration in global config?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

You received this message because you are subscribed to the Google Groups "engineering-code-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email to engineering-code-reviews+unsubscribe@cloudbees.com.
To post to this group, send email to engineering-code-reviews@cloudbees.com.
To view this discussion on the web visit https://groups.google.com/a/cloudbees.com/d/msgid/engineering-code-reviews/jenkinsci/github-plugin/pull/126/c228168540%40github.com.

KostyaSha · 2016-06-23T20:27:46Z

@reviewbybees @stephenc relogin!

stephenc · 2016-06-23T22:20:47Z

Nahh just depends which email I reply to!

stephenc · 2016-06-23T22:21:54Z

Anyway, my phone is the holder of the 2FA for that account...

lanwen · 2016-06-24T09:08:08Z

src/main/java/com/cloudbees/jenkins/GitHubPushTrigger.java

+         * Update the {@link java.util.concurrent.ExecutorService} instance.
+         */
+        /*package*/
+        synchronized void checkThreadPoolSizeAndUpdateIfNecessary() {


it should be a test for this logic

I have lost any energy to continue this PR any further. I want to concentrate my time on the critical root cause, i.e. https://issues.jenkins-ci.org/browse/JENKINS-36147 which once implemented will mean we can rip out this code. If you want to add tests to this code which should be removed once I implement https://issues.jenkins-ci.org/browse/JENKINS-36147, feel free. I will not be updating this PR any further at this point.

It would require updating core that is not good for this plugin. Or we can backport your core improvements after.

well my plan is to make the API available via scm-api without a core bump... and then on newer cores it either will revert to the core implementation (classloader priority) or the scm-api variant will use reflection to redirect to the core implementation

However, it will take me a while to get that API right (as I don't want to end up with the same mistakes of the main Queue API and end up having to have a "big fat lock") so for now, to save users from commit storms killing their Jenkins, this fix would be good to have released.

ookay, i'll merge this. But this is an example of permanent state "will fix that better in future releases". It can't be finished while you've start do things right everywhere

[FIXED JENKINS-36144] Borrow the SCMTrigger's queue (or fall back to …

3b8aaee

…its queue size)

[JENKINS-36144] Ooops need a guard when we can borrow

105ee8f

KostyaSha reviewed Jun 22, 2016
View reviewed changes

stephenc added 2 commits June 22, 2016 12:31

[JENKINS-36144] After careful analysis of some of the code paths, saf…

05b895f

…er not to borrow - we will mirror the count though

[JENKINS-36144] Align with code conventions

3c81c4d

lanwen reviewed Jun 22, 2016
View reviewed changes

stephenc added 2 commits June 23, 2016 00:03

[JENKINS-36144] More verbose function name

3fd5cfe

[JENKINS-36144] Code review catches bugs... who'd have thunk it

dddd6bc

dummy-lanwen-bot reviewed Jun 22, 2016
View reviewed changes

lanwen reviewed Jun 24, 2016
View reviewed changes

lanwen merged commit 10e991e into jenkinsci:master Jun 24, 2016

stephenc deleted the jenkins-36144 branch December 12, 2016 09:08

Conversation

stephenc commented Jun 22, 2016 • edited by KostyaSha Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtnord commented Jun 22, 2016

Uh oh!

rsandell commented Jun 22, 2016

Uh oh!

KostyaSha commented Jun 22, 2016

Uh oh!

KostyaSha commented Jun 22, 2016

Uh oh!

stephenc commented Jun 22, 2016

Uh oh!

lanwen Jun 22, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtnord commented Jun 22, 2016

Uh oh!

ghost commented Jun 22, 2016

Uh oh!

jtnord commented Jun 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dummy-lanwen-bot commented Jun 22, 2016

Uh oh!

KostyaSha commented Jun 23, 2016

Uh oh!

ghost commented Jun 23, 2016

Uh oh!

KostyaSha commented Jun 23, 2016

Uh oh!

stephenc commented Jun 23, 2016

Uh oh!

stephenc commented Jun 23, 2016

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

stephenc commented Jun 22, 2016 •

edited by KostyaSha

Loading

lanwen Jun 22, 2016 •

edited

Loading