-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Save Asynchronous Operations with one Batch improvement #27257
Save Asynchronous Operations with one Batch improvement #27257
Conversation
Hi @nuzil. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
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.
Hi @nuzil!
Thank you for your contribution! It looks really great!
Could you please check my comments?
Also, could you add some tests for introduced behaviour?
/** | ||
* @inheritDoc | ||
*/ | ||
public function execute(array $operations): void |
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.
You have declared OperationInterface[] expected type for '$operations', but in current implementation it will works correctly only with data in arrays.
Please keep contract on OperationInterface or do not make current service as API.
@@ -139,6 +149,7 @@ public function publishMass($topicName, array $entitiesArray, $groupId = null, $ | |||
try { | |||
$operation = $this->operationRepository->createByTopic($topicName, $entityParams, $groupId); | |||
$operations[] = $operation; | |||
$singleOperationsData[] = $operation->getData(); |
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.
To my mind, better is to use DTO from point of view of Service Contracts, than arrays with raw data.
Hi @swnsma, thank you for the review.
|
✔️ QA Passed |
@magento run Functional Tests B2B, Functional Tests EE, Functional Tests CE |
Failed unit tests not related to the changes in this PR |
Hi @nuzil, thank you for your contribution! |
Description (*)
This Pull request is providing performance improvement on a step when Bulk scheduling operation.
Operation are saved into database for reporting, and in existed implementation it happening inside of foreach loop.
So if we have a lot of operations its takes quite a lot of time. Much faster to make it with one Mysql request.
So I chanegd implementation for save results into database after foreach.
Also I made some performance calculation,
with only 200 operations in one request, it gives 20% improvement on performance side.
The bigger amount of operations the bigger performance influence.
Manual testing scenarios (*)
Shedule any bulk operation.
Questions or comments
Auto Tests - implementing of saving to operation table is already covered with tests, so this particular PR do not need any additional tests. If current one will work, then implementation works successfully.
Contribution checklist (*)