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

Permit the setting of Build Schedule Delay to "0". Delay not needed with Replication Events #143

Merged
merged 1 commit into from May 27, 2014

Conversation

Projects
None yet
6 participants
@scoheb
Copy link
Contributor

commented Apr 21, 2014

When using Replication Events, a build schedule delay is not needed since the build will only start once the replication is complete. This PR removes the minimum value 3 seconds BUT keeps the 3 seconds as the default when no value is entered for Build Schedule Delay.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Apr 21, 2014

plugins » gerrit-trigger-plugin #233 FAILURE
Looks like there's a problem with this pull request

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Apr 22, 2014

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@rsandell

This comment has been minimized.

Copy link
Member

commented Apr 22, 2014

This would break some expected behavior by pull #139

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Apr 22, 2014

plugins » gerrit-trigger-plugin #240 FAILURE
Looks like there's a problem with this pull request

@scoheb

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2014

I have added another commit that ensures that we wait at least 3 seconds before checking if the item should block.

hugares referenced this pull request Apr 28, 2014

Updated pom dependency to gerrit-events
Since the module is now released to central
@rsandell

This comment has been minimized.

Copy link
Member

commented Apr 28, 2014

@yannack what do you think?

@yannack

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2014

I haven't pulled and compiled to test, so this is only my opinion based on code reading. The implementation seems good, and provides the "3 second requirement" as part of the dependency extension. This is good as it means the dependency extension does not have expectations on external gerrit-trigger code / behaviour. Also, I think it is nice, since it only enforces this 3 second queuing for projects with dependencies, others may start immediately, so it's a definite improvement overall. 👍

On a side-note: I used "dependency jobs" for "upstream jobs" and "dependent jobs" for "downstream jobs", this latest patch does not, but I think it's not an issue. Moreover, since this naming convention was not very explicit, I only blame myself :) There's a definite vocab issue in English to distinguish between the upstream dependency and the downstream dependent.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented Apr 30, 2014

plugins » gerrit-trigger-plugin #251 FAILURE
Looks like there's a problem with this pull request

@@ -653,7 +653,7 @@ public int getBuildScheduleDelay() {
int max = DEFAULT_BUILD_SCHEDULE_DELAY;
for (GerritServer server : PluginImpl.getInstance().getServers()) {
if (server.getConfig() != null) {
max = Math.max(max, server.getConfig().getBuildScheduleDelay());
max = Math.max(0, server.getConfig().getBuildScheduleDelay());

This comment has been minimized.

Copy link
@rsandell

rsandell May 5, 2014

Member

This is wrong. The algorithm is trying to find the maximum configured schedule delay from all the configured servers. This change would only provide the last configured server's delay.

@rsandell

This comment has been minimized.

Copy link
Member

commented May 5, 2014

The build/test failures are due to this change. Makes me think you never ran them before the push ;)

@hugares

This comment has been minimized.

Copy link
Member

commented May 23, 2014

Hi Robert,
Sorry about this PR...as you noticed, we kind of pushed it before it was ready... I fixed all the issues (checkstyle, missing license, tests,...) and push a new version of the change . I squashed the commits so it is easier to review.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 23, 2014

plugins » gerrit-trigger-plugin #267 UNSTABLE
Looks like there's a problem with this pull request

@hugares

This comment has been minimized.

Copy link
Member

commented May 23, 2014

Latest commit should fix the last test failing, adjusted time out value (was working on my laptop...)

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 23, 2014

plugins » gerrit-trigger-plugin #268 SUCCESS
This pull request looks good

@@ -1 +1,2 @@
<p><strong>Build Schedule Delay</strong> delays the scheduling of the build by the specified number of seconds. The default and minimum <strong>Build Schedule Delay</strong> is 3 seconds. A longer delay can be needed if the changes are to be built from Gerrit mirrors, since the replication takes time.</p>
<p><strong>Build Schedule Delay</strong> delays the scheduling of the build by the specified number of seconds. The default <strong>Build Schedule Delay</strong> is 3 seconds.
A longer delay can be needed if the changes are to be built from Gerrit mirrors, since the replication takes time. However, it you are using replication events to trigger your jobs, you may set this value to 0 to disable the <strong>Build Schedule Delay</strong></p>

This comment has been minimized.

Copy link
@rsandell

rsandell May 26, 2014

Member

it you are using -> if you are using

@rsandell

This comment has been minimized.

Copy link
Member

commented May 26, 2014

Just a tiiiny spelling mistake, otherwise I'm OK to merge.

@hugares

This comment has been minimized.

Copy link
Member

commented May 26, 2014

done

@rsandell

This comment has been minimized.

Copy link
Member

commented May 26, 2014

Are you sure, or is GitHub derping up with "commit --amend" commits again?

Permit the setting of Build Schedule Delay to "0".
Delay not needed with Replication Events. Also fixed dependent projects
feature to no longer rely on this default 3 seconds delay. [FIXED JENKINS-22812]

Change-Id: Ic11f0fb7b9e43e95ae7dec71c30fe1e6fab1e8ac
@hugares

This comment has been minimized.

Copy link
Member

commented May 26, 2014

Sorry, I am on vacation and I tried to fix it quick but I forgot to add file.... ;-)

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 26, 2014

plugins » gerrit-trigger-plugin #272 SUCCESS
This pull request looks good

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 26, 2014

plugins » gerrit-trigger-plugin #273 SUCCESS
This pull request looks good

rsandell added a commit that referenced this pull request May 27, 2014

Merge pull request #143 from scoheb/permit-no-delay
Permit the setting of Build Schedule Delay to "0". Delay not needed with Replication Events

@rsandell rsandell merged commit 38c9d54 into jenkinsci:master May 27, 2014

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.