WIP : Msi stock indexing #48 by larsroettig · Pull Request #49 · magento/inventory

larsroettig · 2017-07-29T14:59:18Z

No description provided.

larsroettig · 2017-07-30T21:33:59Z

@maghamed hi it is only basic version I think must also implement a bunching but the code is working some can be optimized. Have you a good example how can I test it?

maghamed

For Big Merchants, who have a lot of SKUs and a lot of Sales channels (scopes in which products would be sold). We want to prevent a multiplication of data in the index table :
SKUs Qty * Number of Sales Channels

Because that could introduce performance impact both on Read and Write index operations.

That's why it's proposed to split indexes by the Scope.
Taking into account that we have 1 - 1 relation between Scope (Sales Channel) and Stock.
We could Build independent index table per each Stock (Stock_id).

Stock Id could be used as Dimension which will help us to resolve correct index we need to refer to get Product Qty.

For example, having 5 websites with ids (1 .. 5)
we will end up creating 5 different index tables (with the same structure):

inventory_stock_item_stock_1
inventory_stock_item_stock_2
inventory_stock_item_stock_3
inventory_stock_item_stock_4
inventory_stock_item_stock_5

You can refer for similar implementation in the CatalogSearch Index
Magento\CatalogSearch\Model\Indexer\Fulltext
Also, we have a requirement to have index aliasing during the full reindex happened, so we need to have switchable indexes to make Front-End responsive while Full Reindex happen.

Also, please take into account that when the SourceItem data change -> we need to upgrade Stock Item Indexes for each of the Stock to which given Source was assigned.
If Source was assigned to 3 stocks, for example, we need to refresh data in the 3 StockItem indexes

Looks like, for this operation we need to introduce new service
Magento\InventoryApi\Api\GetAssignedStocksForSourceInterface::execute($sourceId);

maghamed · 2017-08-01T13:06:21Z

+use \Psr\Log\LoggerInterface;
+use \Magento\Framework\App\ResourceConnection;
+
+abstract class AbstractAction


Please don't introduce AbstractAction class, as Magento doesn't recommend to use Inheritance in newly created code.

Create a dedicated indexer model which would be injected via DI into each of the class:

Row Rows Full

and make each of these classes to proxy call to that indexer model

maghamed · 2017-08-01T13:25:13Z

+    public function __construct(ResourceConnection $resource, LoggerInterface $logger)
+    {
+        $this->resource = $resource;
+        $this->connection = $resource->getConnection();


it's not recommended to make anything except assignment in the constructors.
Like in your case where you do
$this->connection = $resource->getConnection();

maghamed · 2017-08-01T13:43:51Z

+/**
+ * Stock item Indexer @todo
+ */
+class StockItem implements \Magento\Framework\Indexer\ActionInterface, \Magento\Framework\Mview\ActionInterface


It's wrong to Make indexer implementing both of these interfaces, because these interfaces responsible for different things. Doing so we violate Single Responsibility Principle.

We need to have two different classes, each one implementing own interface.

Like in CatalogSearch module
https://github.com/magento/magento2/blob/develop/app/code/Magento/CatalogSearch/Model/Indexer/Mview/Action.php
implements \Magento\Framework\Mview\ActionInterface
which is declared in - https://github.com/magento/magento2/blob/develop/app/code/Magento/CatalogSearch/etc/mview.xml#L9

and Magento\CatalogSearch\Model\Indexer\Fulltext implements only \Magento\Framework\Indexer\ActionInterface
which is declared in
https://github.com/magento/magento2/blob/develop/app/code/Magento/CatalogSearch/etc/indexer.xml#L9

Let's do the same way

maghamed · 2017-08-01T13:58:14Z

+            $indexItems = $this->fetchIndexItems($sourceItems);
+            if (is_array($indexItems) && count($indexItems) > 0) {
+                $this->cleanIndexTable($sourceItems);
+                $this->updateIndexTable($indexItems);


It's better to introduce implementation of IndexerInterface
https://github.com/magento/magento2/blob/develop/lib/internal/Magento/Framework/Indexer/SaveHandler/IndexerInterface.php

and use it for the operations of clean and delete index

maghamed · 2017-08-01T14:49:09Z

+
+use Magento\InventoryApi\Api\Data\StockItemInterface;
+
+class CreateStockItemIndexTable


This should be implemented similarly to what we have in CatalogSearch
where
Magento\CatalogSearch\Model\Indexer\IndexStructure
responsible for creating StockItem index structure.

IndexStructure implements interface
Magento\Framework\Indexer\IndexStructureInterface

we will reuse this class for index structure creation because we will create dedicated index table for each particular Stock (stock_id). To make the system being scalable horizontally.
As each index table will hold StockItems for each particular Scope (SalesChannel).

For example, having 5 websites each of which represents Sales Channel we will end up with 5 index tables:

inventory_stock_item_index_1 - for website_id 1 inventory_stock_item_index_2 - for website_id 2 inventory_stock_item_index_3 - for website_id 3 inventory_stock_item_index_4 - for website_id 4 inventory_stock_item_index_5 - for website_id 5

maghamed · 2017-08-01T15:08:08Z

+        try {
+            $this->reindexRows($ids);
+        } catch (\Exception $e) {
+            throw new \Magento\Framework\Exception\LocalizedException(__($e->getMessage()), $e);


it's not correct translation,
__($e->getMessage()), $e);
Because \Exception $e ,so it's not a successor of Localized Exception, thus $e->getMessage()
could return a string for which you don't have precise translation

maghamed · 2017-08-01T15:08:29Z

+        try {
+            $this->reindexRows([$id]);
+        } catch (\Exception $e) {
+            throw new \Magento\Framework\Exception\LocalizedException(__($e->getMessage()), $e);


it's not correct translation,
__($e->getMessage()), $e);
Because \Exception $e ,so it's not a successor of Localized Exception, thus $e->getMessage()
could return a string for which you don't have precise translation

maghamed · 2017-08-01T15:10:09Z

+     *
+     * @return void
+     */
+    public function execute($id = null)


why do we allow to accept NULL value if we throw an exception if NULL passed as a parameter?

maghamed · 2017-08-01T15:15:39Z

+ * Interface StockItemInterface
+ * @api
+ */
+interface StockItemInterface extends ExtensibleDataInterface


For now we don't have a Service API to retrieve Stock Items for a given Stock_ID

That's why for now we can't get StockItems in any case
These API should be introduced to InventoryApi\Api\StockItemManagerInterface

maghamed · 2017-08-01T15:16:45Z

+<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Indexer/etc/indexer.xsd">
+    <indexer id="inventory_stock_item" view_id="inventory_stock_item" class="Magento\Inventory\Model\Indexer\StockItem">
+        <title translate="true">StockItem</title>
+        <description translate="true">Index stock item</description>


Please provide more clear description

maghamed

In general everything good, @larsroettig please keep going

maghamed · 2017-08-08T15:34:32Z

no need to make these fields as constants, because constants are the contract which could be accessible outside of the class (like public methods)

it's better to make them private static in this case, so, no one from outside could rely on these values.
And we will not duplicate these data for all instantiated classes.

maghamed · 2017-08-08T15:45:21Z

it's better to use IndexStructureinterface here and specify in DI Type what exact implementation should use here

maghamed · 2017-08-08T15:48:29Z

use DI Type configuration for batchSize, especially taking into account that you will use Type for IndexStructure

maghamed · 2017-08-08T15:53:58Z

hm... it's better to keep this constant away from the concrete implementation, but for now I don't see a place where we could move it

maghamed · 2017-08-08T15:56:40Z

how can we set these data?

maghamed · 2017-08-08T15:59:39Z

let's introduce

interface StockItemIndexerInterface extends \Magento\Framework\Indexer\ActionInterface { /** * Indexer ID in configuration */ const INDEXER_ID = 'inventory_stock_item_index'; }

and make this class to implement this interface.

But Ideally to have getName method in ActionInterface which returned value initialized from the configuration, which we can't add because of BackwardCompatibility

maghamed · 2017-08-08T16:02:31Z

why do we have StockItemManager which behaves like DataInterface,

Manager means to make some actions over some data.

maghamed · 2017-08-08T16:05:29Z

why do we need this DataInterface?
it duplicated StockItemInterface, but called manager and never used

looks like a mistake

naydav · 2017-08-08T15:24:23Z

Cast to int is extra

naydav · 2017-08-08T15:24:39Z

Missed parameter type

naydav · 2017-08-08T15:25:15Z

Pls delete or add more description to @todo annotation

naydav · 2017-08-08T15:26:02Z

Move default value to di configuration

naydav · 2017-08-08T15:27:41Z

Need to more clear name?
What is mean data?

naydav · 2017-08-08T15:31:02Z

As for me *Manager is not good name for "entity"
Maybe it is better simple "StockItem"

But pay attention we already have \Magento\InventoryApi\Api\Data\StockItemInterface

naydav · 2017-08-08T15:34:31Z

it is not stock item sku

naydav · 2017-08-08T15:36:10Z

visa versa
Get Stocks assigned to Source

naydav · 2017-08-08T15:42:30Z

Do we need to process disabling/enabling of products?

ishakhsuvarov · 2017-08-14T15:16:42Z

Please update doc block.

ishakhsuvarov · 2017-08-14T15:18:42Z

Please describe constants.
More info is available in the devdocs

ishakhsuvarov · 2017-08-14T15:19:46Z

Please add a type hint

ishakhsuvarov · 2017-08-14T15:20:03Z

Please add a type hint

maghamed · 2017-08-15T11:41:50Z

It's better to have a separate namespace for Indexer. Not to put all indexers to
Magento\Inventory\Model\*

Let's create dedicated namespace like

Magento\Inventory\Indexer\

maghamed · 2017-08-15T12:21:44Z

In the documentation https://github.com/magento-engcom/magento2/wiki/New-Indexer-for-StockItems#index-dimensions stated

That's why it's proposed to split indexes by the Scope. Taking into account that we have 1 - 1 relation between Scope (Sales Channel) and Stock
For example, having 5 websites with ids (1 .. 5) we will end up creating 5 different index tables (with the same structure):

inventory_stock_item_stock_1 inventory_stock_item_stock_2 inventory_stock_item_stock_3 inventory_stock_item_stock_4 inventory_stock_item_stock_5

Current code where dimension is StoreId is copy-pasted from Search Indexer

maghamed · 2017-08-15T12:46:02Z

we already have
\Magento\Framework\Search\Request\Dimension
why do we create own Dimension object?

naydav · 2017-08-21T11:49:36Z

Pls provide more clear class name
class name is proxy, class interface is resolver, due to code it is factory... it is not clear what is responsibility of this class

naydav · 2017-08-21T11:52:18Z

Do we need to create a new object each time?

naydav · 2017-08-21T11:56:18Z

This logic is better to place in State object. In this way, we don't provide possibility to set the wrong state

But if we look code of State object, we can not find possibility to set the wrong state.
In State objectwWe have only useTemporaryIndex and useRegularIndex methods, thus we can not set state different from these two.
So this checking looks like redundant

agree with @naydav in the scope of UnknownStateException I described the idea of State Machine, so don't think that we need to check the state and have a factory for state creation

naydav · 2017-08-21T12:09:58Z

I understand that is taken from search index but I don't like naming (TemporaryResolver, IndexScopeResolver, ScopeProxy)
@maghamed what do you think about this naming?

@naydav makes sense, naming should be aligned to outline the best practices and make it understandable for external developers what's the responsibility of the class reading its name.

naydav · 2017-08-21T12:11:18Z

Need to think about moving this exception to framework lib if we will use it

@naydav agree, already pointed that out for IndexTableNotExistException.
These should be framework level exceptions, we can't provide them to client because they provide details of how this functionality is implemented (leaky abstractions).

So, these exceptions are not successors of Localized

Btw, I don't know why we have this Exception in our codebase.
So, from what I see we have a pre-defined finite list of states which could be switched in pre-defined order. (aka State machine design pattern)

our state object initially being initialized in 'REGULAR' state.

after that, we've decided to move it to 'TEMPORARY' state while making re-indexation

after re-indexation complete - we move the object to 'REGULAR' state again.

Is it correct? I suppose so, then why do we have a possibility that State object would be created in a wrong state?
@larsroettig

naydav · 2017-08-21T13:28:10Z

Maybe it is better to use one fixture with all preconditions

naydav · 2017-08-21T13:29:09Z

Why do you create an instance of repository each time?

naydav · 2017-08-21T13:30:03Z

You can reuse fixture from catalog module

naydav · 2017-08-21T13:31:33Z

We already have this fixture
app/code/Magento/InventoryApi/Test/_files/source/source_list.php

naydav · 2017-08-21T13:31:50Z

app/code/Magento/InventoryApi/Test/_files/source/stock_list.php

- Add Setup for index table - Add new Interface"

- Refactoring to new Setup structure - Implement SourceItem add entry to di.xml - WIP Register Basic Indexer SourceItem

- Impelemnt basic import funktion TODOS: - Add some bunching function for index - Add Mysql Index to inventory_stock_item_index table - Documentation and clean up

- Clean Up and implement basic structure

- Impelemnt basic import function TODOS: - Index switch from tmp to idx table - Documentation and clean up

- Add Setup for index table - Add new Interface I magento-engcom/magento#48: - Add Setup for index table - Add new Interface

- Implement switch for Index TODO: - Clean Up and more Doc - Discuss the missing website to stock mapping

- Impelemnt Basic intigration test fixtures

- Bugfix for di.xml - Add basic call for reindex full

- Basic Indexer with switch for tmp and normal index TODO: - Refactoring to remove the state folder from the module - Doc blocks and clean up - Add assertions to the test case

-- fixes after merge

magento-cicd2 · 2017-08-22T14:10:49Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ larsroettig
❌ naydav

maghamed

@naydav please help @larsroettig
and join this story development.

It's very sophisticated Story, and there are no good examples in Magento where re-indexation was implemented totally correct.

So, let's make a good example of introducing New Index to the Magento 2. Outlining all the steps and document them afterwards.

We already agreed that ActiveTableSwitcher would be moved to Framework maybe we need some additional things to move to the framework level?

maghamed · 2017-08-23T14:38:17Z

+ * @todo refactoring it copy from catalog search module
+ * @api
+ */
+class IndexTableNotExistException extends LocalizedException


If we are introducing IndexTableNotExistException, this exception should be low-level (framework level).
Not inherited from LocalizedException.
Because we can't provide this Exception o the client code, even its name is a leaky abstraction of the internal details.

maghamed · 2017-08-23T14:43:23Z

@@ -0,0 +1,24 @@
+<?php


In Magento there is another Switcher mechanism - https://github.com/magento-engcom/magento2/blob/develop/app/code/Magento/Catalog/Model/ResourceModel/Indexer/ActiveTableSwitcher.php

by our request, it would be moved to the Framework soon (in the scope of Magento 2.2.0 regression).
So, we could consider using ActiveTableSwitcher for our purposes.

@naydav please consider this and provide a solution for @larsroettig

maybe we need some additional classes to be moved to the Framework?

maghamed · 2017-08-23T15:04:43Z

@naydav makes sense, naming should be aligned to outline the best practices and make it understandable for external developers what's the responsibility of the class reading its name.

maghamed · 2017-08-23T15:06:53Z

@naydav agree, already pointed that out for IndexTableNotExistException.
These should be framework level exceptions, we can't provide them to client because they provide details of how this functionality is implemented (leaky abstractions).

So, these exceptions are not successors of Localized

maghamed · 2017-08-23T15:14:03Z

Btw, I don't know why we have this Exception in our codebase.
So, from what I see we have a pre-defined finite list of states which could be switched in pre-defined order. (aka State machine design pattern)

our state object initially being initialized in 'REGULAR' state.

after that, we've decided to move it to 'TEMPORARY' state while making re-indexation

after re-indexation complete - we move the object to 'REGULAR' state again.

Is it correct? I suppose so, then why do we have a possibility that State object would be created in a wrong state?
@larsroettig

maghamed · 2017-08-23T15:18:41Z

agree with @naydav in the scope of UnknownStateException I described the idea of State Machine, so don't think that we need to check the state and have a factory for state creation

- Add test for index row

# Conflicts: # app/code/Magento/Inventory/etc/di.xml

- refactoring

- fixes

- fix static tests - update db structure

- fix static tests

naydav · 2017-09-01T16:48:28Z

Internal build
https://bamboo.corp.magento.com/browse/M2-AT2478-3

Internal static build (INTEGRITY-SANITY-EE) is red due to MAGETWO-71596
Internal static build (Static LEGACY-EE-L64 and EE-L64) is red.
But coupling between objects for \Magento\Inventory\Test\Integration\Indexer\StockItemTest will be fixed during https://github.com/magento-engcom/magento2/issues/69

maghamed · 2017-09-01T16:56:47Z

Approved to be merged into the Develop branch

Great job guys @larsroettig @naydav , well done!

ACP2E-3863: [Bundle 12] Update copyrights for MSI and fix static errors.

larsroettig changed the title ~~WIP : Msi stock indexing~~ WIP : Msi stock indexing #48 Jul 30, 2017

larsroettig mentioned this pull request Jul 30, 2017

Stock item indexer #48

Closed

maghamed suggested changes Aug 1, 2017

View reviewed changes

maghamed suggested changes Aug 8, 2017

View reviewed changes

naydav reviewed Aug 9, 2017

View reviewed changes

ishakhsuvarov reviewed Aug 14, 2017

View reviewed changes

Comment thread app/code/Magento/Inventory/Model/Indexer/StockItem.php Outdated

Copy link
Copy Markdown

Contributor

ishakhsuvarov Aug 14, 2017

Choose a reason for hiding this comment

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

Please update doc block.

ishakhsuvarov reviewed Aug 14, 2017

View reviewed changes

maghamed reviewed Aug 15, 2017

View reviewed changes

naydav reviewed Aug 21, 2017

View reviewed changes

naydav force-pushed the develop branch from 9627b00 to a64ea9b Compare August 22, 2017 09:52

larsroettig added 17 commits August 22, 2017 17:00

MSI magento-engcom/magento#48:

512901b

- Add Setup for index table - Add new Interface"

MSI magento-engcom/magento#48:

e6188e5

- Refactoring to new Setup structure - Implement SourceItem add entry to di.xml - WIP Register Basic Indexer SourceItem

MSI magento-engcom/magento#48:

d420097

- Impelemnt basic import funktion TODOS: - Add some bunching function for index - Add Mysql Index to inventory_stock_item_index table - Documentation and clean up

Add bugfix for tests

d607cf4

Remove setup

3fa9e32

MSI magento-engcom/magento#48:

1ebb5b7

- Clean Up and implement basic structure

Add basic structure

886edf6

Implement mview

39edf65

WIP

178439f

wip

9ac0c36

MSI magento-engcom/magento#48:

201c60f

- Impelemnt basic import function TODOS: - Index switch from tmp to idx table - Documentation and clean up

MSI magento-engcom/magento#48:

223e3b9

- Add Setup for index table - Add new Interface I magento-engcom/magento#48: - Add Setup for index table - Add new Interface

MSI magento-engcom/magento#48:

bddf88c

- Implement switch for Index TODO: - Clean Up and more Doc - Discuss the missing website to stock mapping

wip

91c2aea

MSI magento-engcom/magento#48:

f9945c4

- Impelemnt Basic intigration test fixtures

MSI magento-engcom/magento#48:

8ee0afe

- Bugfix for di.xml - Add basic call for reindex full

MSI magento-engcom/magento#48:

859a726

- Basic Indexer with switch for tmp and normal index TODO: - Refactoring to remove the state folder from the module - Doc blocks and clean up - Add assertions to the test case

MSI: WIP : Msi stock indexing

31e5df0

-- fixes after merge

naydav force-pushed the msi-stock-indexing branch from 23864df to 31e5df0 Compare August 22, 2017 14:10

maghamed reviewed Aug 23, 2017

View reviewed changes

larsroettig and others added 15 commits August 26, 2017 23:05

MSI magento-engcom/magento#48:

e5f0b21

- Add test for index row

Merge remote-tracking branch 'origin/develop' into msi-stock-indexing

09cb38b

# Conflicts: # app/code/Magento/Inventory/etc/di.xml

WIP: Msi stock indexing #49

6abbedd

- refactoring

WIP: Msi stock indexing #49

d09d345

- refactoring

WIP: Msi stock indexing #49

9d184c2

- refactoring

WIP: Msi stock indexing #49

953705f

- refactoring

WIP: Msi stock indexing #49

de908a8

- refactoring

WIP: Msi stock indexing #49

e563f96

- refactoring

WIP: Msi stock indexing #49

ee79fd4

- refactoring

WIP: Msi stock indexing #49

0dc8be6

- refactoring

WIP: Msi stock indexing #49

bdb20ed

- refactoring

WIP: Msi stock indexing #49

10b7933

- fixes

WIP: Msi stock indexing #49

e08a7a4

- fixes

WIP: Msi stock indexing #49

5820160

- fix static tests - update db structure

WIP: Msi stock indexing #49

555cfb9

- fix static tests

naydav merged commit b5c9550 into develop Sep 1, 2017

vrann added msi and removed msi labels Sep 29, 2017

maghamed added Award: advanced Award: special achievement labels Oct 2, 2017

smoskaluk mentioned this pull request Mar 23, 2018

Exception in Low stock report for product with another name on additional storeview #737

Closed

maghamed deleted the msi-stock-indexing branch December 11, 2018 18:12

magento-devops-reposync-svc pushed a commit that referenced this pull request Jun 5, 2025

Merge pull request #49 from adobe-commerce-tier-4/ACP2E-3863-Bundle12

247f658

ACP2E-3863: [Bundle 12] Update copyrights for MSI and fix static errors.


		use Magento\InventoryApi\Api\Data\StockItemInterface;

		class CreateStockItemIndexTable

Conversation

larsroettig commented Jul 29, 2017

Uh oh!

larsroettig commented Jul 30, 2017

Uh oh!

maghamed left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maghamed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naydav Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maghamed left a comment •

edited

Loading

naydav Aug 8, 2017 •

edited

Loading