Created memory efficient iterator for importexport_importdata table #10918
Conversation
|
|
| /** | ||
| * Iterator constructor. | ||
| * @param \Magento\Framework\Model\ResourceModel\Db\Context $context | ||
| * @param null $connectionName |
There was a problem hiding this comment.
Type hint for this parameter should be string as in parent constructor
| ) { | ||
| parent::__construct($context, $connectionName); | ||
|
|
||
| $this->rowsIds = $this->getRowsIds(); |
There was a problem hiding this comment.
According to Magento Technical Guidelines it is strongly discouraged to perform any operations in the constructor of the class with the exception of Dependency Assignment and Dependency Validation operations.
Implementing business logic inside the constructor violates single-responsibility principle, generally makes code more error-prone and harder to test.
| /** | ||
| * Import data iterator | ||
| */ | ||
| class Iterator extends \Magento\Framework\Model\ResourceModel\Db\AbstractDb |
There was a problem hiding this comment.
Looks like this iterator may be decomposed into actual iterator and resource model which provides data to the iterator. Current implementation has multiple responsibilities which does not follow SOLID principles.
| } | ||
|
|
||
| return $iterator; | ||
| return $this->iteratorFactory->create(); |
There was a problem hiding this comment.
Iterator factory should produce \Iterator objects accodring to the method docblock
| /** | ||
| * Import data iterator | ||
| */ | ||
| class Iterator extends \Magento\Framework\Model\ResourceModel\Db\AbstractDb |
There was a problem hiding this comment.
This class should implement \Iterator
…nts from PR. Fixed unit tests
| * @param string $connectionName | ||
| * @param array $arguments | ||
| */ | ||
| public function __construct( | ||
| \Magento\Framework\Model\ResourceModel\Db\Context $context, | ||
| \Magento\Framework\Json\Helper\Data $jsonHelper, | ||
| \Magento\ImportExport\Model\ResourceModel\Import\Data\IteratorFactory $iteratorFactory, |
There was a problem hiding this comment.
See http://devdocs.magento.com/guides/v2.0/contributor-guide/backward-compatible-development/#adding-a-constructor-parameter for a correct way to add new dependency.
| /** | ||
| * DataProvider for import data | ||
| */ | ||
| class DataProvider extends \Magento\Framework\Model\ResourceModel\Db\AbstractDb |
There was a problem hiding this comment.
Please name classes properly, not a word-per-folder.
| /** | ||
| * Import data iterator | ||
| */ | ||
| class Iterator implements \Iterator |
There was a problem hiding this comment.
Please name classes properly, not a word-per-folder.
|
Hi @diwipl, are you going to continue work on this PR? |
|
Yes, I will. Sorry for the delay. |
|
Closing due to inactivity. Feel free to reach me out anytime later if you wish to continue work on pull request and it will be reopened. |
Default iterator for importexport_importdata table was loading whole table contents to memory, resulting in huge memory usage when importing large numbers of products.
Description
Additional iterator was created. It gets all rows ids from table and then uses them to return "current" row in memory optimized way.
Fixed Issues (if relevant)
Manual testing scenarios
Contribution checklist