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.

Sign up for GitHub

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

Jump to bottom

Parallelize scenario migrations testing #5327

Merged
merged 10 commits into from
Jan 18, 2024
Merged

Parallelize scenario migrations testing #5327

merged 10 commits into from
Jan 18, 2024

Conversation

Elmacioro
Copy link
Contributor

Describe your changes

Checklist before merge

  • Related issue ID is placed at the beginning of PR title in [brackets] (can be GH issue or Nu Jira issue)
  • Code is cleaned from temporary changes and commented out lines
  • Parts of the code that are not easy to understand are documented in the code
  • Changes are covered by automated tests
  • Showcase in dev-application.conf added to demonstrate the feature
  • Documentation added or updated
  • Added entry in Changelog.md describing the change from the perspective of a public distribution user
  • Added MigrationGuide.md entry in the appropriate subcategory if introducing a breaking change
  • Verify that PR will be squashed during merge

@github-actions github-actions bot mentioned this pull request Jan 9, 2024
Closed
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jan 9, 2024 •
edited
Loading

created: #5376
⚠️ Be careful! Snapshot changes are not necessarily the cause of the error. Check the logs.

@Elmacioro Elmacioro marked this pull request as ready for review January 9, 2024 12:05
@Elmacioro
Copy link
Contributor Author

After consulting with @piotrp I will also:

  • parallelize fetchGroupByGroup operation
  • Modify logging to show how many threads we're using for testing migration
  • Add some meaningful prefixes for ForkJoinPool threads so it looks better in thread dumps/ profiler

@Elmacioro Elmacioro force-pushed the parallelize-model-migration-test branch from 9f7ad76 to 7c50847 Compare January 10, 2024 15:27
@github-actions github-actions bot mentioned this pull request Jan 10, 2024
Closed
@Elmacioro
Copy link
Contributor Author

I will abstain from merging for the time being as I'll cherry-pick commits from this PR to preview/esp_1.12.4 branch and test the changes.

@Elmacioro
Copy link
Contributor Author

I updated PR to use two ExecutionContexts instead of one as using only one EC with strictly restricted parallelism made things significantly worse performance-wise during tests. The new EC will be only used for operations on parallel collections while the old implicit one will take care of the tasks that it used to.

@Elmacioro Elmacioro force-pushed the parallelize-model-migration-test branch from 8b8f92e to 00896fb Compare January 16, 2024 10:17
@Elmacioro
Copy link
Contributor Author

In the latest commit I modified fetchGroupByGroup to be executed in parallel bounding the number of maximum concurrent requests to the number of threads in the batchingExecutionContext. Previous approach did not work as I intended since foldLeft forced fetching to be executed in sequence even when called on parallel collection.

@github-actions github-actions bot mentioned this pull request Jan 16, 2024
Closed
arkadius
arkadius approved these changes Jan 18, 2024
View reviewed changes
Copy link
Member

@arkadius arkadius left a comment

Choose a reason for hiding this comment

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

LGTM

@Elmacioro Elmacioro merged commit 7b22541 into staging Jan 18, 2024
16 checks passed
@Elmacioro Elmacioro deleted the parallelize-model-migration-test branch January 18, 2024 09:56
mslabek pushed a commit that referenced this pull request Jan 21, 2024
@Elmacioro @mslabek
Parallelize scenario migrations testing (#5327)
3072f9c 
* Parallelize scenario migrations testing

* Remove unused ExecutionContext from HttpRemoteEnvironment

* Parallelize fetchGroupByGroup operation

* Add prefixes for custom batching ExecutionContext

* Remove using explicit ExecutionContext for testing migration

* Inline ExecutionContextTaskSupport instantiation

* Add spearate ExecutionContext for operations on collections

* Remove implicit from TestModelMigrationsSpec

* Fix fetchGroupByGroup parallelization

* Move batchTimeout to config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@piotrp piotrp piotrp approved these changes

@arkadius arkadius arkadius approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Elmacioro @arkadius @piotrp