-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Introduces the use of GoodJob::Batch
for CopyProjectJob
#15054
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mereghost
commented
Mar 20, 2024
modules/storages/app/workers/storages/copy_project_folders_job.rb
Outdated
Show resolved
Hide resolved
mereghost
force-pushed
the
impl/split-storage-jobs
branch
from
March 21, 2024 13:13
1391c36
to
8856f81
Compare
mereghost
force-pushed
the
impl/batch_copy_project_job
branch
2 times, most recently
from
March 21, 2024 16:33
5185bfb
to
cb0fe34
Compare
mereghost
force-pushed
the
impl/batch_copy_project_job
branch
4 times, most recently
from
March 25, 2024 15:18
5725a25
to
8d7a24e
Compare
mereghost
force-pushed
the
impl/split-storage-jobs
branch
from
March 25, 2024 15:36
8856f81
to
1aa09a2
Compare
mereghost
force-pushed
the
impl/batch_copy_project_job
branch
from
March 25, 2024 16:07
8d7a24e
to
5bdd1ca
Compare
mereghost
requested review from
cbliard,
ulferts,
Kharonus,
oliverguenther and
a team
March 25, 2024 17:53
mereghost
force-pushed
the
impl/batch_copy_project_job
branch
from
March 29, 2024 09:01
d25eb60
to
6145d8d
Compare
mereghost
force-pushed
the
impl/batch_copy_project_job
branch
from
April 2, 2024 16:53
6145d8d
to
761cc4b
Compare
akabiru
approved these changes
Apr 3, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨ Amazing feat @mereghost 👏🏾 and as always, thank you for the detailed PR description! 👍🏾
✅ Code looks great to me- added Qs and discussion points
📆 Will reach out to you for some smoke testing
...pp/common/storages/peripherals/storage_interaction/nextcloud/copy_template_folder_command.rb
Outdated
Show resolved
Hide resolved
...pp/common/storages/peripherals/storage_interaction/nextcloud/copy_template_folder_command.rb
Show resolved
Hide resolved
...ages/app/common/storages/peripherals/storage_interaction/result_data/copy_template_folder.rb
Show resolved
Hide resolved
modules/storages/app/services/storages/file_links/copy_file_links_service.rb
Outdated
Show resolved
Hide resolved
modules/storages/app/services/storages/file_links/copy_file_links_service.rb
Outdated
Show resolved
Hide resolved
modules/storages/app/workers/storages/copy_project_folders_job.rb
Outdated
Show resolved
Hide resolved
...mmon/storages/peripherals/storage_interaction/nextcloud/copy_template_folder_command_spec.rb
Show resolved
Hide resolved
modules/storages/spec/services/storages/project_storages/copy_project_folders_service_spec.rb
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the following Work Packages: OP#53036 and OP#53035
What is this?
It all started when we decided to add
OneDrive/Sharepoint
as part of the File Storages offering…It was a rainy night at a dive bar in downtown BerSupporting OneDrive/Sharepoint (OD for brevity sake) has presented us with new challenges, from what was the actual concept of a
FileStorage
to implementation details such as permission handling et al. It works so differently than our other use case (Nextcloud) that we had to adapt our codebase and software behavior to make it conform to both.This is one of those places: copying a project or using it as a template. This PR aims to implement this feature for OD.
Why do we need this?
OD, when it comes to copying folders is an asynchronous call. We ask for the copy, they gives an URL to check the status of the copy job and at the end handles us the new
resource_id
. Compared to the synchronous nature of Nextcloud, this was quite the change.So the code would need to be bent and reshaped to our needs: we'd need to poll the OD provided URL until it is done, which of course presents its own challenges.
During this time we had
GoodJob
Pavel merged giving us the ability to run jobs in batches, which fits this type of work as a glove.How does it work?
Part 1: Batches 101
To make a long topic short:
GoodJob::Batch
coordinates jobs enqueued under it. It also makes possible to share some state between the jobs, by the use orBatch#properties
and to add other jobs as callbacks to batch events (such as finished, discarded etc).To enqueue a new
Batch
:The batch properties are serialized by
ActiveJob::Serializers
so anything that's safe for a job, is safe for it.From inside a job, if you have included
GoodJob::ActiveJobExtensions::Batches
you can access the batch by simply callingbatch
.But that's not all! Batches can be updated in runtime, adding new jobs or properties to it as demonstrated above. Cool, huh?
Part 2: The current state
Here is a small list of all the pieces that already exist that needed some tuning or revamps.
Project::CopyService
: This is were the bulk of the action happens.Project::CopyService
uses the structure of theDependentServices
to handle all its intricacies.CopyProjectJob
: This is where we ensure that everything non-copying is done, like making sure we respect the user localization and sending e-mails . It also is the job that gets polled by the UI for copy completion.StoragesDependentService
,StorageProjectFoldersDependentService
,FileLinksDependentService
: this is where most of the work forStorages
happen, making sure that the particularities of each storage and project storage mode are respected.These pieces worked together to provide the the entirety of the Copy Folder feature taking into account the Nextcloud integration.
Part 3: What changed (aka the cool part):warning:
First thing was to break most of the behavior for copying project folders from
CopyService
. This created 2 services:ProjectStorages::CopyProjectFoldersService
andFileLinks::CopyFileLinksService
. Those 2 new services take over the oldDependent
services but provide basically the same functionality.But
CopyService
uses the.copy_dependencies
method as a base to tell the UI what can be copied, so here it comes the first jury-rig of the PR.copyable_dependencies
was overridden and the entries relating toFileLinks
andProjectFolders
added by hand (aka copy-pasted) from the now deprecated services.But now we have to deal with "maybe polling", so a new background job comes in:
CopyProjectFoldersJob
. The main point of this job is to retry itself if aPollingRequired
exception is raised. This will happen if the result of theCopyFolderCommand
indicates that it requires polling.So, now we need a way to coordinate the
CopyProjectJob
with one or moreCopyProjectFoldersJob
, that's where theGoodJob::Batch
comes in. Most of the changes will affect theCopyProjectJob
. I'll add some snippets, but please check out the code.CopyProjectJob
Batch editionAfter cleaning up a bit the instance variables (it had like 11 million of them), we hook it to the batch system. We also want to enqueue the
CopyProjectFoldersJob
from within it, adding them to the batch. But how? LuckilyCopyProject
keeps a state with a lot of information about what has transpired internally so that otherDependent
services can rely on previous data.So right after the copy process concludes, we tap into that state (conveniently called
#state
), and look for the key that we are interested in:copied_project_storages
. This carries the source and copiedProjectStorage
pairs, so looping through this list we add a newCopyProjectFoldersJob
for each copiedProjectStorage
, passing along some extra needed information.Now that we have the batch, we can rely on it to store the polling URLs and the state of the polling process without the need for error prone
Thread.current
values.We also only want to e-mail the user once the entire process completes, so another job was born
SendCopyProjectStatusEmailJob
. This one is added as a callback that is called once the entire batch finishes (successfully or not). Sounds easy enough, but it needs a bunch of information (errors, copied project name etc) so we push all this info to theBatch#properties
making easy this new job easy to access the necessary info (or any other jobs in the batch).Ok, cool. Which are the gotchas here?
Since our API doesn't know how to poll for
Batch
completion the user will be redirected once theCopyProjectJob
is done even if theCopyProjectFoldersJob
are enqueued/running. The completion email, will only be sent once everything is done.The amount of change would not be trivial, so I left this as a future improvement (future can be tomorrow 🤣).
Storages
aren't also writing to theerrors
right now as we still need to decide on how to deal withcommand
s that might error successfully (or some operations might fail, but overall be considered a success).