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

feat: Dynamic parallelism mechanism for all UploadQueues #1472

Merged
merged 29 commits into from
Mar 26, 2025
Merged

feat: Dynamic parallelism mechanism for all UploadQueues #1472

merged 29 commits into from
Mar 26, 2025

Conversation

adrien-coye
Copy link
Contributor

@adrien-coye adrien-coye commented Mar 14, 2025 •
edited
Loading

Since we now have two upload queues, each queue is no longer managing its own degree of parallelism.

A new UploadParallelismOrchestratorobject now dynamically computes the optimal queue depth for each queue in a way that makes it equivalent to the previous mechanism with one queue only.

Depends on Infomaniak/swift-dependency-injection#12
Depends on #1478

@adrien-coye adrien-coye force-pushed the feat/upload-parallelism branch 2 times, most recently from 28dfffb to 23d32ef Compare March 14, 2025 08:13
@adrien-coye adrien-coye changed the title refactor(UploadService): Parallelism observation feat(UploadParallelismOrchestrator): Parallelism mechanism for all UploadQueues Mar 14, 2025
@adrien-coye adrien-coye changed the title feat(UploadParallelismOrchestrator): Parallelism mechanism for all UploadQueues feat: Dynamic parallelism mechanism for all UploadQueues Mar 14, 2025
This was referenced Mar 14, 2025
Merged
Open
@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from 5e164fd to 9c276f5 Compare March 18, 2025 12:14
@adrien-coye adrien-coye force-pushed the feat/upload-parallelism branch from 6725743 to a82e1f7 Compare March 18, 2025 12:45
adrien-coye
adrien-coye commented Mar 19, 2025
View reviewed changes
Copy link
Contributor Author

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

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

Yesterday I thoroughly tested this branch. I'm happy with the way it behaves. I also don't see anything in the code that need to be changed, so opening for review.

@adrien-coye adrien-coye marked this pull request as ready for review March 19, 2025 06:09
@github-actions GitHub Actions
Copy link

This PR/issue depends on:

@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from 7515d01 to aa860d2 Compare March 20, 2025 07:49
@adrien-coye adrien-coye force-pushed the feat/upload-parallelism branch from 30a2822 to 28019f1 Compare March 20, 2025 07:52
PhilippeWeidmann
PhilippeWeidmann reviewed Mar 20, 2025
View reviewed changes
Copy link
Member

@PhilippeWeidmann PhilippeWeidmann left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code looks okay apart from one minor comment let me know what you think.
I just need to play a little bit with it.

@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from aa860d2 to 5b2de0f Compare March 21, 2025 06:46
@adrien-coye adrien-coye force-pushed the feat/upload-parallelism branch from 4492c9f to 93e733b Compare March 21, 2025 06:47
@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from 5b2de0f to 877a325 Compare March 24, 2025 12:42
@adrien-coye adrien-coye force-pushed the feat/upload-parallelism branch from 337b977 to cd2779c Compare March 24, 2025 12:43
adrien-coye
adrien-coye commented Mar 24, 2025
View reviewed changes
Copy link
Contributor Author

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

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

Did a new self review

@adrien-coye adrien-coye force-pushed the feat/upload-parallelism branch from e25d8eb to 285c296 Compare March 24, 2025 13:03
adrien-coye
adrien-coye commented Mar 25, 2025
View reviewed changes
Copy link
Contributor Author

@adrien-coye adrien-coye left a comment

Choose a reason for hiding this comment

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

I did a new self review

@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from 877a325 to f2604f5 Compare March 26, 2025 07:24
@adrien-coye
fix(WorkloadParallelismHeuristic): Remove side effect at init that wo…
cb16c56 
…uld break all the current code
@adrien-coye
refactor(UploadQueue): New isActive property
04f1cec
@adrien-coye
feat(UploadParallelismOrchestrator): Implementation of the dynamic up…
7927909 
…load parallelism algo per queue
@adrien-coye
feat(ParallelismDefaults): Enum to clarify default values
480262f
@adrien-coye
fix(WorkloadParallelismHeuristic): Make sure to compute a first time …
eab1046 
…a parallelism value outside of the DI queue
@adrien-coye
fix(UploadService): Make sure to rebuild the upload queues on init ou…
f77b276 
…tside the DI queue
@adrien-coye
fix(UploadQueueObserver): Only notify operationQueueNoLongerEmpty whe…
499b265 
…n needed
@adrien-coye
refactor(UploadParallelismOrchestrator): Switched set logic for simpl…
a1e9344 
…er filter function
@adrien-coye
refactor(UploadService): Dedicated queues for events and rebuilding t…
19e83c9 
…he upload queues
@adrien-coye
refactor(UploadService): Explicit blocking and non blocking rebuildUp…
420fd57 
…loadQueue func
@adrien-coye
refactor(WorkloadParallelismHeuristic): Internal serial event queue
f816a62
@adrien-coye
refactor(UploadParallelismOrchestrator): Internal serial event queue
a08c87e
@adrien-coye
fix(WorkloadParallelismHeuristic): Double call to computeParallelism
4301669
@adrien-coye
refactor(UploadParallelismOrchestrator): Notify memory pressure on an…
354178a 
… internal queue
@adrien-coye
refactor(UploadQueueObserver): Notify observation on an internal queue
9e2540c
@adrien-coye
refactor(UploadQueueObserver): Observation setup in a dedicated metho…
eabe82d 
…d to clean a Sonar warning
@adrien-coye
refactor(UploadService): Removed extraneous lazy in DI call
c95f736
@adrien-coye
refactor(UploadQueueObserver): Split some existing code to reduce com…
9cdd583 
…plexity and a Sonar warning
@adrien-coye
refactor(UploadQueueObserver): Stateless implementation
d84dd54
@adrien-coye
chore: Fix name of function in log event
0acebfb
@adrien-coye
refactor(UploadService): Using internal serial queue for complex tran…
a3faa7f 
…sactions
@adrien-coye
refactor(UploadService): Simpler init since the called method runs on…
1910c76 
… a dedicated queue
@adrien-coye
refactor(UploadService): New setupObservation function
fe72c15
@adrien-coye
refactor(UploadParallelismOrchestrator): New setupObservation function
f0fae91
@adrien-coye
chore(UploadService): Removed comment
ba9d70a
@adrien-coye adrien-coye force-pushed the feat/upload-parallelism branch from a442d06 to ba9d70a Compare March 26, 2025 07:24
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@adrien-coye adrien-coye merged commit 3e24922 into feat/photo-upload-queue Mar 26, 2025
5 checks passed
@adrien-coye adrien-coye deleted the feat/upload-parallelism branch March 26, 2025 08:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilippeWeidmann PhilippeWeidmann PhilippeWeidmann approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@adrien-coye @PhilippeWeidmann