Remove the concept of yanking dead executors #2440

Merged
merged 2 commits into from Jul 8, 2016

Conversation

5 participants
@jglick
Member

jglick commented Jul 8, 2016

Historically when Executor.run failed with an exception, the executor was marked as “dead” and there was a yank command to restore it, after showing some UI asking you to report the error.

This was always pretty terrible UI. Normally Queue.Executable implementations are designed to return normally from run(). For example, even if an AbstractBuild is badly broken, Run.execute(RunExecution) aggressively catches errors of all kinds, and either prints them to the build log, or sends them to the Jenkins log if the build log is not yet initialized or already finalized. So as a practical matter, the only time you were likely to have a RuntimeException thrown up from run() was if Queue.Task.createExecutable() failed—for example, with the IllegalStateException corrected (for at least some cases) in #2439. But these cases are almost certainly core or plugin bugs, not build misconfigurations. And the stack trace was already being printed to the Jenkins log, just as it would be for errors thrown from dozens of other extension point implementations.

So why was this case being treated differently, with a Jenkins administrator required to view the exception before being allowed to proceed using the executor again? Probably because in the old days, Executors were a more or less fixed asset of Computers, reused for each build, so if one died (the thinking went—@kohsuke?) you should have to think about replacing it. But as of #944 even this justification makes no sense: for years now, every Executor is used just once, then thrown out, with the Computer refilling its pool automatically according to the configured count. So given that the error was already being reported to the log, there is just no reason to block that normal recycling. It only annoys users who encounter something like JENKINS-27530 because they are forced to take action before Jenkins works again.

@reviewbybees

@reviewbybees

This comment has been minimized.

Show comment
Hide comment
@reviewbybees

reviewbybees Jul 8, 2016

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.

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.

@abayer

This comment has been minimized.

Show comment
Hide comment
@abayer

abayer Jul 8, 2016

Member

🐝 Death to dead executors! That always baffled me in practice.

Member

abayer commented Jul 8, 2016

🐝 Death to dead executors! That always baffled me in practice.

@rsandell

This comment has been minimized.

Show comment
Hide comment
@rsandell

rsandell Jul 8, 2016

Member

🐝 Indeed, kill them with fire! :)

Member

rsandell commented Jul 8, 2016

🐝 Indeed, kill them with fire! :)

@jglick jglick merged commit c528cdc into jenkinsci:master Jul 8, 2016

1 check passed

Jenkins This pull request looks good
Details

@jglick jglick deleted the jglick:no-dead-executors branch Jul 8, 2016

jglick added a commit that referenced this pull request Jul 8, 2016

*/
- @RequirePOST

This comment has been minimized.

@oleg-nenashev

oleg-nenashev Jul 9, 2016

Member

I would not remove RequirePOST in such case, because it only makes the deprecated method more available 🐛

@oleg-nenashev

oleg-nenashev Jul 9, 2016

Member

I would not remove RequirePOST in such case, because it only makes the deprecated method more available 🐛

This comment has been minimized.

@jglick

jglick Jul 25, 2016

Member

So what? It does nothing, so there is no need for @RequirePOST.

@jglick

jglick Jul 25, 2016

Member

So what? It does nothing, so there is no need for @RequirePOST.

@oleg-nenashev

This comment has been minimized.

Show comment
Hide comment
@oleg-nenashev

oleg-nenashev Jul 9, 2016

Member

👎 for merging it without letting other contributors to review it

Member

oleg-nenashev commented Jul 9, 2016

👎 for merging it without letting other contributors to review it

oleg-nenashev added a commit that referenced this pull request Jul 15, 2016

[FIXED JENKINS-33201 Java JSON exception with an empty parametrized b…
…uild.] (#2444)

* Fixed JENKINS-33201 [Java JSON exception with an empty parametrized
build.]

* FIXED JENKINS-33201. Changed to FormException instead of Failure

* That fix was never in a released version.

So it cannot have been reverted.

* Lowercase i for consistency

* Remove the concept of yanking dead executors.

* Another unnecessary call to doYank.

* [FIXED JENKINS-27530] Jenkins.reload must also reload the Queue to ensure that every Queue.Item.task corresponds to a live Job, lest nextBuildNumber be bogus.

* [JENKINS-27530] Noting #2439 & #2440.

* FIXED JENKINS-33201. Changed to FormException instead of Failure

* [FIXED JENKINS-33201 .FormException is thrown if parameterDefinitions are not available .

* Changing one more Tab to Space

* Moved checkForEmptyParameters to Job class
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment