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

RateLimitBranchProperty: Fix checking queue for other items tied to the same task #383

Merged
merged 3 commits into from
Jun 26, 2023

Conversation

Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Jun 21, 2023

getId is not comparable, but getInQueueSince is.

This also adds a LoggerRule to the test class to make the test output more readable.

cc @jglick

Testing done

Submitter checklist

… to the same task

There is no need for a for loop when only checking the first item would be enough.
@Vlatombe Vlatombe requested a review from a team as a code owner June 21, 2023 16:24
@jglick jglick changed the title RateLimitBranchProperty: simplify checking queue for other items tied to the same task. RateLimitBranchProperty: simplify checking queue for other items tied to the same task Jun 26, 2023
@jglick jglick added the bug label Jun 26, 2023
@Vlatombe Vlatombe requested a review from jglick June 26, 2023 14:16
@Vlatombe Vlatombe changed the title RateLimitBranchProperty: simplify checking queue for other items tied to the same task RateLimitBranchProperty: Fix checking queue for other items tied to the same task Jun 26, 2023
@@ -537,7 +537,7 @@ public CauseOfBlockage canRun(Queue.Item item) {
return null;
}
for (Queue.Item i : items) {
if (i.getId() < item.getId()) {
if (i.getInQueueSince() < item.getInQueueSince()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now printing

foo/master with queue id 4 blocked by queue id 3 was first

as expected.

@jglick jglick enabled auto-merge June 26, 2023 15:16
@jglick jglick merged commit 5d691a8 into jenkinsci:master Jun 26, 2023
13 checks passed
@Vlatombe Vlatombe deleted the simplify-rate-limit-queue-check branch June 26, 2023 15:56
@jglick
Copy link
Member

jglick commented Jun 26, 2023

I suspect you need a short pause between

QueueTaskFuture<FreeStyleBuild> future = master.scheduleBuild2(0);
QueueTaskFuture<FreeStyleBuild> future2 = master.scheduleBuild2(0, (Cause) null,
new ParametersAction(
Collections.singletonList(new StringParameterValue("FOO", "MANCHU"))));
to ensure that the items have distinct start times, since this is only millisecond resolution; or
if (i.getInQueueSince() < item.getInQueueSince()) {
should use <= while checking (&&) that the Items or their ids are !=.

@Vlatombe
Copy link
Member Author

The latter makes more sense to me since in theory this condition can still happen (if jobs are triggered via a script).

#385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants