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.

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

Fix mass copy/paste async task #582

Merged
merged 3 commits into from
Mar 26, 2024
Merged

Fix mass copy/paste async task #582

merged 3 commits into from
Mar 26, 2024

Conversation

miknevinas
Copy link
Contributor

The mass paste task wasn't working due to some batching logic - the idea was to commit a transaction for every 50 documents moved. However, about every other commit threw a Zope 'ConflictError', and after the third batch the process would quit and nothing would be committed.

I tried a few of the built-in Zope solutions for avoiding transaction conflicts (transaction managers, the 'attempts' mechanism to retry transactions, etc) but none of these could circumvent the error. The ZODB docs highly discourage the developer from trying to take conflict resolution into their own hands, so I ultimately decided to just remove the logic entirely (as it was not present in the mass delete task).

I tested the task by pasting up to around 2000 documents and everything works fine - no conflict errors arise.

@bduncan137
Copy link
Collaborator

removing the batching will likely slow this down a bit, but that might not be a big deal since the batching is in a worker and the user isn't waiting anyway as i understand it. As i recall, an email is supposed to be generated and sent to a user if there are a large number of items in copy/paste (i think?) If i'm not wrong on that, did you confirm that the email is generated and sent as anticipated?

@bduncan137
Copy link
Collaborator

we'll need a pr into 2.x as well, i think

@miknevinas
Copy link
Contributor Author

miknevinas commented Jan 18, 2024

removing the batching will likely slow this down a bit, but that might not be a big deal since the batching is in a worker and the user isn't waiting anyway as i understand it. As i recall, an email is supposed to be generated and sent to a user if there are a large number of items in copy/paste (i think?) If i'm not wrong on that, did you confirm that the email is generated and sent as anticipated?

Speed was about the same when I tested it with some escape logic that would just skip the batch if there was a conflict, so I think it should be fine there.

The email is now sent as well, it wasn't sending before as process would just freeze after a couple of failed transactions.

Copy link
Collaborator

@bduncan137 bduncan137 left a comment

Choose a reason for hiding this comment

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

I see nothing obviously wrong here, but i'm curious to see zombified review this too

@miknevinas
Copy link
Contributor Author

I've revisited this and re-run a bunch of tests. It takes about 60 seconds to paste 500 items. I've looked at a bunch of 'Press Releases' directories, and most of them have anywhere between 500 - 2500 documents, so each one should only take a few minutes (ironically, it takes half that time to simply 'Select All' in these views due to the sheer number of documents).

I tested pasting up to 5000 documents and the results seem to scale linearly. The task runs successfully even at that volume, taking about 12 minutes.

I also experimented with batching - batches larger than about 10 fail after three commits due to a Zope conflict error, even if the batch sizes are large. Anything smaller appears to overload redis memory and crashes after a dozen or so commits.

This is the only instance of transaction batching I could find in Castle - even the 'mass delete' task does it all in one transaction, so I'm pretty confident that these changes should be safe.

To pursue this further, we would need to try manual conflict resolution, which Zope highly discourages in the docs.

@zombified zombified merged commit 2331ac7 into master Mar 26, 2024
@zombified zombified deleted the am/copy-paste-task branch March 26, 2024 13:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants