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

MSI magento-engcom/magento#91: #106

Merged
merged 21 commits into from
Oct 4, 2017
Merged

MSI magento-engcom/magento#91: #106

merged 21 commits into from
Oct 4, 2017

Conversation

larsroettig
Copy link
Member

@larsroettig larsroettig commented Sep 24, 2017

#Performance Optimizing for the Indexing Mview Part #91

  • Fix IndexHandler Contract in the Implementation
  • Fix Documentations

@vrann vrann added msi and removed msi labels Sep 29, 2017
@@ -22,6 +22,19 @@
public function saveIndex(IndexName $indexName, \Traversable $documents);

/**
* Create the index i not exits or remove sku list from the index to rebuild
Copy link
Contributor

Choose a reason for hiding this comment

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

why method, which name is cleanUp creates a table?

* @param string $connectionName
* @return void
*/
public function cleanUp(
Copy link
Contributor

Choose a reason for hiding this comment

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

let's rename the method into cleanIndex
as we have saveIndex and deleteIndex methods here

* Returns all assigned stock ids by given sources item ids.
*
* @param int[] $sourceItemIds
* @return int[] List of stock id to sku1,sku2 assignment
Copy link
Contributor

Choose a reason for hiding this comment

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

PHP Annotation for type is wrong, because it's not an array of int[]

it's an array of arrays , like

[1] => ['sku-1', 'sku-2', sku-3],
[2] => ['sku-1']

maybe it's better to use an object to keep the contract of result

/**
* Returns all assigned stock ids by given sources ids
*/
class GetDeltaReindexData
Copy link
Contributor

Choose a reason for hiding this comment

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

let's re-name Delta -> Partial

@@ -79,7 +88,7 @@ public function __construct(
*/
public function executeFull()
{
$stockIds = $this->getAssignedStockIds->execute([]);
$stockIds = $this->getFullReindexData->execute();
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a bug in executeFull indexing implementation.
Currently it looks like

    public function executeFull()
    {
        $stockIds = $this->getFullReindexData->execute();

        foreach ($stockIds as $stockId) {
            $mainIndexName = $this->indexNameBuilder
                ->setIndexId(StockItemIndexerInterface::INDEXER_ID)
                ->addDimension('stock_', $stockId)
                ->setAlias(Alias::ALIAS_MAIN)
                ->build();
            $this->indexStructure->create($mainIndexName);

            $replicaIndexName = $this->indexNameBuilder
                ->setIndexId(StockItemIndexerInterface::INDEXER_ID)
                ->addDimension('stock_', $stockId)
                ->setAlias(Alias::ALIAS_REPLICA)
                ->build();
            $this->indexStructure->create($replicaIndexName);

But IndexStructure::create works like

   public function create(IndexName $indexName, string $connectionName = ResourceConnection::DEFAULT_CONNECTION)
    {
        $connection = $this->resourceConnection->getConnection($connectionName);
        $tableName = $this->indexNameResolver->resolveName($indexName);

        if ($connection->isTableExists($tableName)) {
            $connection->truncateTable($tableName);
            return;
        }

        $this->createTable($connection, $tableName);

So, it Truncates table if it exists.

The main reason to use _Main table and _Replica was to support 0 downtime.
And make a re-indexation on _Replica table while _Main table is being accessed and after re-indexation to rename _Replica -> _Main.
But currently at the beggining of the re-indexation we just truncate both of the tables

$connection = $this->resourceConnection->getConnection($connectionName);
$tableName = $this->indexNameResolver->resolveName($indexName);
if ($connection->isTableExists($tableName) === false) {
$this->createTable($connection, $tableName);
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect you will get Fatal Error at this line

- fixes for truncate table
- Refactoring for complex array to domain model
@maghamed maghamed deleted the msi-index-91 branch December 11, 2018 18:15
magento-cicd2 pushed a commit that referenced this pull request Apr 8, 2021
[Arrows] MC-40738: [MSI] “Add to Compare” function is not available if a Product is unavailable in the “Default Source” or unassigned from it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants