FEAT: Parallelize the creation and addition of builders to the Pipeline. #160
Conversation
Unit Tests - Ubuntu_AnyCPU_Debug 10 files ± 0 10 suites ±0 1m 29s ⏱️ -8s For more details on these failures, see this check. Results for commit a997fb4. ± Comparison against base commit 51e6b47. This pull request removes 8 and adds 16 tests. Note that renamed tests count towards both. |
Unit Tests - macos_AnyCPU_Debug52 tests 52 ✅ 2m 3s ⏱️ Results for commit a997fb4. |
Unit Tests - macos_AnyCPU_Release0 tests 0 ✅ 0s ⏱️ Results for commit a997fb4. |
Integration Tests - macos_AnyCPU_Debug0 tests 0 ✅ 0s ⏱️ Results for commit a997fb4. |
Integration Tests - macos_AnyCPU_Release0 tests 0 ✅ 0s ⏱️ Results for commit a997fb4. |
There was a problem hiding this comment.
I think the order of the elements no longer corresponds to the order of them in the configuration, and the test failures might be indicating it too: https://github.com/51Degrees/pipeline-dotnet/pull/160/checks?check_run_id=34471477263
Please check the comments, and make sure the order is preserved and the tests pass.
| @@ -402,7 +410,7 @@ private void AddElementToList( | |||
| /// The index of the element within the <see cref="PipelineOptions"/>. | |||
| /// </param> | |||
| private void AddParallelElementsToList( | |||
There was a problem hiding this comment.
This is cosmetics, but I would rename this method into EnqueParallelElements since now it is not adding to the list but adding to the queue
| @@ -222,7 +230,7 @@ private void GetAvailableElementBuilders() | |||
| /// <see cref="PipelineOptions"/> instance. | |||
| /// </param> | |||
| private void AddElementToList( | |||
There was a problem hiding this comment.
This is cosmetics, but I would rename this method into EnqueueElement now since it is no longer adding to the list, but adding to the queue.
|
|
||
| // Add created builders to flow elements | ||
| FlowElements.AddRange(flowElementQueue); |
There was a problem hiding this comment.
We constructed the elements in parallel and they were added in the chronological order of construction completion into the flowElementQueue, but we expect that they should be added into the FlowElements in the order in which they were specified in the configuration, not in the chronological order of construction completion... So perhaps we need to associate an index with each of them during construction - to place into the right position..
… limit the processor count so that it doesnt use up all resources on start up.
…ch parallel interation and insert each element at the respective index. This preserves the element order that is declared in the configuration.
… List runs into Out of Index exceptions because of how lists are created.
There was a problem hiding this comment.
- It seems there are no order-checking tests. We probably need to add some and verify that the order of elements corresponds to the one in the configuration.
- One more issue that we run into with this change is that we are breaking the exception contract. If you run tests locally (which you should do in any case before pushing code, to make sure they pass) - you will find that they fail, because
ForEachthrows anAggregateException, while the tests expectPipelineConfigurationException. This actually leads to another problem - that ForEach will construct all of the elements - regardless of whether any of them have failed... While it would be ideal to cancel the construction when at least one of them have failed... So we have to tie together catching the exception, firing the cancellation token, rethrowing the exception. This may become a pretty complicated implementation, but we need to implement it in the way that if one of the parallel tasks fails - we don't complete (or start if we haven't yet started) all others.
No description provided.