-
-
Notifications
You must be signed in to change notification settings - Fork 650
Implement transactions #3814
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
Implement transactions #3814
Conversation
68087a3
to
dd388df
Compare
@nadvornik I would just throw an error in case you attempt to edit the same item in parallel and ignore that request for the transaction in question. If you are using Cobbler as a "library" and do multithreaded operations you should synchronize these operations by yourself IMHO. The solution for environments like this would be to wait until the conflicting transaction is finished and re-evaluate if the desired action still makes sense to be performed. |
e9e7cd8
to
986b531
Compare
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferencesCodacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more |
986b531
to
65e470e
Compare
65e470e
to
5f6bc66
Compare
I have updated the tests to cover a scenario where a profile is reparented and the old parent is deleted in one transaction. |
@nadvornik Currently the CI reports the following issues:
|
5f6bc66
to
414a9ae
Compare
@nadvornik I have taken the liberty to rebase the PR and fix black and the changelog. Please fix pyright! |
@nadvornik Could you get this finished before the end of the week please? I would like to backport it to 3.3.7 and include it with the release that will be made on Sunday. We have a community member asking for this to be available. |
04792a1
to
d7eaf8a
Compare
@SchoolGuy Linter is fixed. Is there anything else that needs to be improved? |
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.
Thank you for this lengthy process. I will run the CI now and if all checks out we can merge the PR!
@nadvornik pyright is reporting a lot of low-hanging fruits for the code you wrote. While I would accept some errors (which we would need to silence with |
d62f134
to
5930744
Compare
@SchoolGuy pyright should be fixed now |
@nadvornik Now you broke Black and isort, and you lost the changes I force pushed that fix the GH actions workflows. |
@SchoolGuy please try again |
@nadvornik You still need to restore the changes I force pushed that enable the CI to run again... Edit: The changes only affect the workflow configuration for GitHub. |
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.
The current CI failures that are left are not related to this PR. Let's get it merged.
Linked Items
Fixes #3810
Description
This is an implementation of transactions, as discussed in the related issue.
It fixes the original problem - the speed of creating and deleting of 500 profiles is now under 30s, from the original ~1hour.
Behaviour changes
Old: No change.
New: Added optional XMLRPC calls begin_transaction(token) and commit_transaction(token)
The modify/save/remove/copy/new operations performed between these calls are stored in memory and are visible
only for the client identified by the token. commit_transaction saves everything and regenerates the menus in one call.
Unresolved issues / TODO
It is not possible to create parent-child relation without saving the parent first.Conflicts when the same items are modified in parallel in multiple transactions.Tested only with profiles, so far.Category
This is related to a:
Tests