implement optional concurrency limit for parallel blocks #57
Conversation
|
Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests |
|
This change cannot be merged as-is. The change needs to be documented; and there should be tests as well. |
|
I will add some documentation, however, I couldn't come up with any ideas for a test case that wouldn't be more more complicated than the actual code change that would actually validate the concurrency limit. If you'll accept a test that simply tests that the parameter is accepted, I'll add that. Otherwise, I'm open to ideas. |
jcarrothers-sap
force-pushed
the
limitedParallel
branch
from
f519216
to
99044be
Compare
jcarrothers-sap
changed the title
|
/poke |
|
maybe you could use a semaphore and |
|
Perhaps you can provide a little more detail? I'm unfamiliar with the method calls that you mention. Upon further thought, I'm also not convinced that a test of the actual limit functionality is really worthwhile since the code simply relies on the underlying implementation of Executors.newFixedThreadPool(). Perhaps, if there is an easy way to mock and monitor this method call, but I don't see any precedence for that in the other tests. Perhaps I missed an example? |
…el blocks
The closure parameters to a parallel block call can now be pre-pended by a
number which is interpreted as a limit on the number of closure blocks to be
executed in parallel.
Example: parallel(2, {build("job1")}, {build("job2")}, {build("job3")})
The above would execute up to two of the specified jobs simultaneously.
The test testParallelLimitConcurrency now does a very basic test of the concurrency limit by using a job which pauses for ten seconds and thus the expected runtime of the flow can be tested based on the configured limit.
jcarrothers-sap
force-pushed
the
limitedParallel
branch
from
99044be
to
44a9940
Compare
|
As much as I hate to cause the tests take even longer to complete... :( |
It was possible for for the killRunningJobs method to run into a ConcurrentModificationException when a flow has parallel blocks due to new build blocks starting, and thus adding entries the jobsGraph as killRunningJobs is attempting to abort the flow and all of its downstream jobs. This error was always possible, if a flow with parallel blocks was aborted at just the right moment, but the limitel concurrency feature makes it slightly more likely.
|
I had forgotten Java provided a semaphore implementation, I've updated the tests to use that instead as well as merging in the latest master to resolve a merge conflict. |
This should prevent testThatAbortAbortsStartedJobs_IgnoredParallel from occasionally failing when it shouldn't.
|
Is there anything further necessary in order to get this pull request merged? |
| @@ -1,8 +1,9 @@ | |||
| /* | |||
| * The MIT License | |||
| * | |||
| * Copyright (c) 2013, CloudBees, Inc., Nicolas De Loof. | |||
| * Cisco Systems, Inc., a California corporation | |||
| * Copyright (c) 2013-2015, CloudBees, Inc., Nicolas De Loof. | |||
There was a problem hiding this comment.
please feel free to add a but I'm hesitant about changing existing notices.
There was a problem hiding this comment.
Because it is basically impossible to separate the copyright ownership of the various pieces, I feel that a single date range and a list of the contributes makes more sense. The original work was done in 2015, but I will update due to review changes.
|
@jcarrothers-sap, Side note: I think introducing new features in this plugin is risky and takes away resources that could be better spent on the pipeline plugin. However, please feel free to merge if you are interested in carrying on the flag. |
The closure parameters to a parallel block call can now be pre-pended by a
number which is interpreted as a limit on the number of closure blocks to be
executed in parallel.
Example: parallel(2, {build("job1")}, {build("job2")}, {build("job3")})
The above would execute up to two of the specified jobs simultaneously.