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: PhotoSync is performed on a dedicated UploadQueue #1471

Merged
merged 41 commits into from
Mar 26, 2025
Merged

feat: PhotoSync is performed on a dedicated UploadQueue #1471

merged 41 commits into from
Mar 26, 2025

Conversation

adrien-coye
Copy link
Contributor

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

Necessary changes in the UploadService to enable a dedicated UploadQueue for photoSync.

Depends on #1478
Depends on #1472

@adrien-coye adrien-coye mentioned this pull request Mar 12, 2025
Merged
@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from 8f4d3db to b4aa0ee Compare March 13, 2025 13:42
@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from 40989d9 to 5e164fd Compare March 14, 2025 06:45
adrien-coye
adrien-coye commented Mar 14, 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 self review, tested this, at this point it works fine

@adrien-coye adrien-coye marked this pull request as ready for review March 14, 2025 07:01
PhilippeWeidmann
PhilippeWeidmann requested changes Mar 14, 2025
View reviewed changes
Copy link
Member

@PhilippeWeidmann PhilippeWeidmann left a comment

Choose a reason for hiding this comment

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

I don't think it's the UploadFile's responsability to choose the queue where it belongs.
The UploadService already knows the queues and should have the logic.

Something like queueFor(uploadFile: UploadFile) -> UploadQueueable
What do you think about it?

Base automatically changed from feat/split-upload-queue to master March 14, 2025 10:40
@adrien-coye
Copy link
Contributor Author

I don't think it's the UploadFile's responsability to choose the queue where it belongs.

@PhilippeWeidmann In the spirit of the current codebase, having this as a decoration of UploadFile was a pragmatic thing I did. This type already does a lot of things it should not be responsible for.

Yeah I agree with you, since you want it I'll make sure to do it 👍

@adrien-coye adrien-coye mentioned this pull request Mar 17, 2025
Open
@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from 5e164fd to 9c276f5 Compare March 18, 2025 12:14
PhilippeWeidmann
PhilippeWeidmann approved these changes Mar 19, 2025
View reviewed changes
Copy link
Member

@PhilippeWeidmann PhilippeWeidmann left a comment

Choose a reason for hiding this comment

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

Mergeable once dependencies are also merged 👍

@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch 4 times, most recently from 877a325 to f2604f5 Compare March 26, 2025 07:24
@adrien-coye adrien-coye mentioned this pull request Mar 26, 2025
Closed
1 task
@github-actions GitHub Actions
Copy link

This PR/issue depends on:

1 similar comment
@github-actions GitHub Actions
Copy link

This PR/issue depends on:

@PhilippeWeidmann PhilippeWeidmann self-requested a review March 26, 2025 08:32
PhilippeWeidmann
PhilippeWeidmann approved these changes Mar 26, 2025
View reviewed changes
Copy link
Member

@PhilippeWeidmann PhilippeWeidmann 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 last test with the final branch. LGTM 👍

@adrien-coye
fix(WorkloadParallelismHeuristic): Remove side effect at init that wo…
0130b06 
…uld break all the current code
@adrien-coye
refactor(UploadQueue): New isActive property
e99a320
@adrien-coye
feat(UploadParallelismOrchestrator): Implementation of the dynamic up…
1522f34 
…load parallelism algo per queue
@adrien-coye
feat(ParallelismDefaults): Enum to clarify default values
11e1dc5
@adrien-coye
fix(WorkloadParallelismHeuristic): Make sure to compute a first time …
dde5af6 
…a parallelism value outside of the DI queue
@adrien-coye
fix(UploadService): Make sure to rebuild the upload queues on init ou…
52c8dea 
…tside the DI queue
@adrien-coye
fix(UploadQueueObserver): Only notify operationQueueNoLongerEmpty whe…
5387875 
…n needed
@adrien-coye
refactor(UploadParallelismOrchestrator): Switched set logic for simpl…
1be17de 
…er filter function
@adrien-coye
refactor(UploadService): Dedicated queues for events and rebuilding t…
bdd7429 
…he upload queues
@adrien-coye
refactor(UploadService): Explicit blocking and non blocking rebuildUp…
04700a0 
…loadQueue func
@adrien-coye
refactor(WorkloadParallelismHeuristic): Internal serial event queue
cbfeb6d
@adrien-coye
refactor(UploadParallelismOrchestrator): Internal serial event queue
c160220
@adrien-coye
fix(WorkloadParallelismHeuristic): Double call to computeParallelism
d760e68
@adrien-coye
refactor(UploadParallelismOrchestrator): Notify memory pressure on an…
6f07562 
… internal queue
@adrien-coye
refactor(UploadQueueObserver): Notify observation on an internal queue
897fa4d
@adrien-coye
refactor(UploadQueueObserver): Observation setup in a dedicated metho…
6874d48 
…d to clean a Sonar warning
@adrien-coye
refactor(UploadService): Removed extraneous lazy in DI call
5900b8e
@adrien-coye
refactor(UploadQueueObserver): Split some existing code to reduce com…
3b73213 
…plexity and a Sonar warning
@adrien-coye
refactor(UploadQueueObserver): Stateless implementation
8d1fbd6
@adrien-coye
chore: Fix name of function in log event
52d4235
@adrien-coye
refactor(UploadService): Using internal serial queue for complex tran…
cdae43a 
…sactions
@adrien-coye
refactor(UploadService): Simpler init since the called method runs on…
5b57dd6 
… a dedicated queue
@adrien-coye
refactor(UploadService): New setupObservation function
65f69d0
@adrien-coye
refactor(UploadParallelismOrchestrator): New setupObservation function
2959b83
@adrien-coye
chore(UploadService): Removed comment
fe6ad03
@adrien-coye adrien-coye force-pushed the feat/photo-upload-queue branch from 3e24922 to fe6ad03 Compare March 26, 2025 08:35
@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 added this to the 5.3.1 milestone Mar 26, 2025
@adrien-coye adrien-coye merged commit 77c7956 into master Mar 26, 2025
8 checks passed
@adrien-coye adrien-coye deleted the feat/photo-upload-queue branch March 26, 2025 09:46
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
5.3.1
Development

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