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

Use the standard Timer rather than our own ScheduledExecutorService #84

Merged
merged 3 commits into from
Nov 3, 2021

Conversation

jglick
Copy link
Member

@jglick jglick commented Oct 19, 2018

Analogous to jenkinsci/workflow-durable-task-step-plugin#81 but this time reverting a bit of complexity in #60. Together they seem to solve the RestartableJenkinsRule problems in jenkinsci/ant-plugin#32. Timer should work fine in agent JVMs (it does not reference Jenkins model classes); the difference is for durable tasks (in watch mode) run on the master node: Timer gets properly shut down when Jenkins exits, and is thus safe for in-process restart tests. Again unlikely to have any meaningful impact in production.

Copy link
Member

@svanoort svanoort left a comment

Choose a reason for hiding this comment

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

See my comments on #81 for why this is not the best solution and probably should be accomplished via shutdown hook / Terminator.

Again, I'd hesitate to trim back the thread pools that we have created to protect as Bulkheads to protect the (heavily used) Jenkins Timer thread pool from misbehaving Pipeline work -- especially when that work is tied to network operations that carry extra risks.

Now, if we made the Timer pool more robust and able to scale up/down, that might be another story.

@jglick
Copy link
Member Author

jglick commented Oct 26, 2018

I'd hesitate to trim back the thread pools that we have created to protect as Bulkheads to protect the (heavily used) Jenkins Timer thread pool from misbehaving Pipeline work

I think you misunderstand. This is watch-mode code, running in the agent JVM. (Assuming you are running the node block on an agent. If you are not, who cares about scalability?) So this is not the same Timer pool.

Removes the code added in jenkinsci#86 which is not needed here.
@jglick jglick dismissed svanoort’s stale review April 12, 2019 16:14

already responded

@jglick
Copy link
Member Author

jglick commented Apr 24, 2019

CI flakiness…

@jglick jglick closed this Apr 24, 2019
@jglick jglick reopened this Apr 24, 2019
@jglick jglick requested a review from car-roll November 3, 2021 19:39
@car-roll car-roll merged commit adeab44 into jenkinsci:master Nov 3, 2021
@jglick jglick deleted the standard-thread-pool branch November 3, 2021 23:06
@jglick
Copy link
Member Author

jglick commented Nov 15, 2021

BTW @car-roll this had no label, so appears without category in https://github.com/jenkinsci/durable-task-plugin/releases/tag/493.v195aefbb0ff2. Need to make sure to apply one before merging. There is probably some GH Action out there which enforces this…if you can find one which works reliably, is easy to configure, and works properly with forks (should I guess just add a failing Check if there is no label?), then please suggest an addition to the recommended workflow skeleton for JEP-229 in https://github.com/jenkinsci/.github/tree/master/workflow-templates (CC @timja).

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