Skip to content
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

[FIXED JENKINS-28446] - proper calculation of queue length in UnlabeledLoadStatistics #1709

Merged
merged 4 commits into from May 21, 2015

Conversation

3 participants
@oleg-nenashev
Copy link
Member

commented May 18, 2015

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

UnlabeledLoadStatistics:: computeQueueLength() was broken starting from the first release (reason - wrong assumption due to the undocumented behavior of Queue::countBuildableItemsFor()). There's n impact on cloud provisioners if a job has no label definitions.

The fix enforces a filtering of queue items with labels. Backporting to the current LTS is required IMO.

@reviewbybees @recampbell

oleg-nenashev added some commits May 18, 2015

Code documentation/annotation around JENKINS-28446.
Just to prevent similar misuses in the future.
[FIXED JENKINS-28446] - Calculate only tasks without assigned labels
This implementation does not create new methods in API, hence it can be backported.
@stephenc

This comment has been minimized.

Copy link
Member

commented May 18, 2015

You've implemented the old API not the new one

@stephenc

This comment has been minimized.

Copy link
Member

commented May 18, 2015

So what you really need to do is fix Queue.countBuildableItemsFor(null)... which sadly has a dual usage mode. I suspect we want to introduce Queue.strictCountBuildableItemsFor(label) as the existing one has additional symantics for null arguments

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented May 18, 2015

You've implemented the old API not the new one

The old API is still being by external callers. The "modern" API will be use filters (which are correct) or be defaulted to the call I've fixed

So what you really need to do is fix Queue.countBuildableItemsFor(null)... which sadly has a dual usage mode. I suspect we want to introduce Queue.strictCountBuildableItemsFor(label) as the existing one has additional symantics for null arguments

This change is definitely useful as an extra patch, but it does not replace the fix for the Old API, which should be back-portable to 1.609

@stephenc

This comment has been minimized.

Copy link
Member

commented May 18, 2015

You should not be iterating the Queue from the load stats

You should be using the snapshot in Queue.

af55e53 was the commit that broke countBuildableItemsFor in this regard

@stephenc

This comment has been minimized.

Copy link
Member

commented May 18, 2015

By doing the iteration in ULS you force the allocation of a new array list. Much better to just have a correct method in Queue and call that. Some investigation is required to determine why af55e53 needed to count tasks with no assigned label as being valid for a specific label

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented May 18, 2015

You should not be iterating the Queue from the load stats. You should be using the snapshot in Queue.

Queue::getBuildableItems() returns an array from the snapshot, hence it should be OK.

Some investigation is required to determine why af55e53 needed to count tasks with no assigned label as being valid for a specific label

I suppose the committer just wanted to use this method in Queue::countBuildableItems()

By doing the iteration in ULS you force the allocation of a new array list. Much better to just have a correct method in Queue and call that.

I'll introduce such API method. BTW, the current implementation should be backported to the previous LTS, hence I propose to add a separate commit w/o squashing it

@stephenc

This comment has been minimized.

Copy link
Member

commented May 18, 2015

Well if you want to backport a fix it should be a different fix for pre 1.607

Don't mess up the tuned code in 1.607+ just because you want to backport the fix to pre 1.607. Create a pull request against the specific branch and fix master the right way

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented May 18, 2015

Well if you want to backport a fix it should be a different fix for pre 1.607

It will be back-ported to 1.609.x, so there's no need in the completely different fix

@stephenc

This comment has been minimized.

Copy link
Member

commented May 18, 2015

Then fix it right. Don't count in the ULS count in Queue and just invoke the method direct to save spinning out an Array List

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented May 18, 2015

@stephenc
Done

@stephenc

This comment has been minimized.

Copy link
Member

commented May 18, 2015

👍 but I would prefer if we could find out why countBuildabelItemsFor needs the special counting of items with a null label

@oleg-nenashev

This comment has been minimized.

Copy link
Member Author

commented May 18, 2015

@stephenc
I suppose in af55e53 @aklochkovgd used such hack to calculate all buildable items. This hack has not been noticed by PR reviewers, and the change has been merged. There was no buzz regarding regressions after the merge.

Any hack becomes a "feature" after 2 years, so I would not revert the existing behavior

@stephenc

This comment has been minimized.

Copy link
Member

commented May 18, 2015

Well it would be good to document why in the code

@tfennelly

This comment has been minimized.

Copy link
Member

commented May 18, 2015

👍 in so much as I get the problem

@stephenc

This comment has been minimized.

Copy link
Member

commented May 19, 2015

👍

oleg-nenashev added a commit that referenced this pull request May 21, 2015

Merge pull request #1709 from oleg-nenashev/JENKINS-28446-fix
[FIXED JENKINS-28446] - proper calculation of queue length in UnlabeledLoadStatistics

@oleg-nenashev oleg-nenashev merged commit 7205da0 into jenkinsci:master May 21, 2015

1 check passed

Jenkins This pull request looks good
Details

oleg-nenashev added a commit that referenced this pull request May 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.