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

WIP : Msi stock indexing #48 #49

Merged
merged 33 commits into from
Sep 1, 2017
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
512901b
MSI magento-engcom/magento#48:
Jul 23, 2017
e6188e5
MSI magento-engcom/magento#48:
Jul 29, 2017
d420097
MSI magento-engcom/magento#48:
Jul 30, 2017
d607cf4
Add bugfix for tests
Jul 30, 2017
3fa9e32
Remove setup
Aug 5, 2017
1ebb5b7
MSI magento-engcom/magento#48:
Aug 5, 2017
886edf6
Add basic structure
Aug 6, 2017
39edf65
Implement mview
Aug 7, 2017
178439f
WIP
Aug 12, 2017
9ac0c36
wip
Aug 12, 2017
201c60f
MSI magento-engcom/magento#48:
Aug 12, 2017
223e3b9
MSI magento-engcom/magento#48:
Aug 12, 2017
bddf88c
MSI magento-engcom/magento#48:
Aug 12, 2017
91c2aea
wip
Aug 15, 2017
f9945c4
MSI magento-engcom/magento#48:
Aug 15, 2017
8ee0afe
MSI magento-engcom/magento#48:
Aug 15, 2017
859a726
MSI magento-engcom/magento#48:
Aug 20, 2017
31e5df0
MSI: WIP : Msi stock indexing
Aug 22, 2017
e5f0b21
MSI magento-engcom/magento#48:
Aug 26, 2017
09cb38b
Merge remote-tracking branch 'origin/develop' into msi-stock-indexing
Aug 29, 2017
6abbedd
WIP: Msi stock indexing #49
Aug 29, 2017
d09d345
WIP: Msi stock indexing #49
Aug 31, 2017
9d184c2
WIP: Msi stock indexing #49
Aug 31, 2017
953705f
WIP: Msi stock indexing #49
Aug 31, 2017
de908a8
WIP: Msi stock indexing #49
Aug 31, 2017
e563f96
WIP: Msi stock indexing #49
Aug 31, 2017
ee79fd4
WIP: Msi stock indexing #49
Aug 31, 2017
0dc8be6
WIP: Msi stock indexing #49
Aug 31, 2017
bdb20ed
WIP: Msi stock indexing #49
Aug 31, 2017
10b7933
WIP: Msi stock indexing #49
Sep 1, 2017
e08a7a4
WIP: Msi stock indexing #49
Sep 1, 2017
5820160
WIP: Msi stock indexing #49
Sep 1, 2017
555cfb9
WIP: Msi stock indexing #49
Sep 1, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public function execute()
'[ID: %value] %message',
[
'value' => $itemData[SourceInterface::SOURCE_ID],
'message' => $e->getMessage()
'message' => $e->getMessage(),
]
);
}
Expand Down
76 changes: 76 additions & 0 deletions app/code/Magento/Inventory/Indexer/Scope/IndexSwitcher.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Inventory\Indexer\Scope;


use Magento\Framework\App\ResourceConnection;
use Magento\Framework\Search\Request\IndexScopeResolverInterface;

/**
* Provides a functionality to replace main index with its temporary representation
*
* @todo refactoring it copy from catalog search module
*/
class IndexSwitcher implements IndexSwitcherInterface
{
/**
* @var Resource
*/
private $resource;

/**
* @var ScopeProxy
*/
private $resolver;

/**
* @var State
*/
private $state;

/**
* @param ResourceConnection $resource
* @param IndexScopeResolverInterface $indexScopeResolver
* @param State $state
*/
public function __construct(
ResourceConnection $resource,
IndexScopeResolverInterface $indexScopeResolver,
State $state
) {
$this->resource = $resource;
$this->resolver = $indexScopeResolver;
$this->state = $state;
}

/**
* {@inheritdoc}
* @throws IndexTableNotExistException
*/
public function switchIndex(array $dimensions, $index)
{
if (State::USE_TEMPORARY_INDEX === $this->state->getState()) {
$temporalIndexTable = $this->resolver->resolve($index, $dimensions);
if (!$this->resource->getConnection()->isTableExists($temporalIndexTable)) {
throw new IndexTableNotExistException(
__(
"Temporary table for index $index doesn't exist,"
. " which is inconsistent with state of scope resolver"
)
);
}

$this->state->useRegularIndex();
$tableName = $this->resolver->resolve($index, $dimensions);
if ($this->resource->getConnection()->isTableExists($tableName)) {
$this->resource->getConnection()->dropTable($tableName);
}

$this->resource->getConnection()->renameTable($temporalIndexTable, $tableName);
$this->state->useTemporaryIndex();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
namespace Magento\Inventory\Indexer\Scope;

/**
* Provides a functionality to replace main index with its temporary representation
* @todo refactoring it copy from catalog search module
*/
interface IndexSwitcherInterface
{
/**
* Switch current index with temporary index
*
* It will drop current index table and rename temporary index table to the current index table.
*
* @param array $dimensions
* @param string $name of the indexer
* @return void
*/
public function switchIndex(array $dimensions, $index);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Inventory\Indexer\Scope;

use Magento\Framework\Exception\LocalizedException;

/**
* Exception which represents situation where temporary index table should be used somewhere,
* but it does not exist in a database
* @todo refactoring it copy from catalog search module
* @api
*/
class IndexTableNotExistException extends LocalizedException
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
}
76 changes: 76 additions & 0 deletions app/code/Magento/Inventory/Indexer/Scope/ScopeProxy.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Inventory\Indexer\Scope;

use Magento\Framework\Search\Request\Dimension;

/**
* Implementation of IndexScopeResolverInterface which resolves index scope dynamically
* depending on current scope state
* @todo refactoring it copy from catalog search module
*/
class ScopeProxy implements \Magento\Framework\Search\Request\IndexScopeResolverInterface
Copy link

Choose a reason for hiding this comment

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

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

{
/**
* Object Manager instance
*
* @var \Magento\Framework\ObjectManagerInterface
*/
private $objectManager;

/**
* @var array
*/
private $states = [];

/**
* @var State
*/
private $scopeState;

/**
* Factory constructor
*
* @param \Magento\Framework\ObjectManagerInterface $objectManager
* @param State $scopeState
* @param array $states
*/
public function __construct(
\Magento\Framework\ObjectManagerInterface $objectManager,
State $scopeState,
array $states
) {
$this->objectManager = $objectManager;
$this->scopeState = $scopeState;
$this->states = $states;
}

/**
* Creates class instance with specified parameters
*
* @param string $state
* @return \Magento\Framework\Search\Request\IndexScopeResolverInterface
* @throws UnknownStateException
*/
private function create($state)
{
if (!array_key_exists($state, $this->states)) {
throw new UnknownStateException(__("Requested resolver for unknown indexer state: $state"));
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

}
return $this->objectManager->create($this->states[$state]);
Copy link

Choose a reason for hiding this comment

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

Do we need to create a new object each time?

}

/**
* @param string $index
* @param Dimension[] $dimensions
* @return string
*/
public function resolve($index, array $dimensions)
{
return $this->create($this->scopeState->getState())->resolve($index, $dimensions);
}
}
58 changes: 58 additions & 0 deletions app/code/Magento/Inventory/Indexer/Scope/State.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Inventory\Indexer\Scope;

/**
* This class represents state that defines which table should be used during indexation process
*
* There are two possible states:
* - use_temporary_table
* - use_main_table
*
* The 'use_main_table' state means that default indexer table should be used.
*
* The 'use_temporary_table' state is an opposite for 'use_main_table'
* which means that default indexer table should be left unchanged during indexation
* and temporary table should be used instead.
* @todo refactoring it copy from catalog search module
*/
class State
{
const USE_TEMPORARY_INDEX = 'use_temporary_table';
const USE_REGULAR_INDEX = 'use_main_table';

/**
* @var string
*/
private $state = self::USE_REGULAR_INDEX;

/**
* Set the state to use temporary Index
* @return void
*/
public function useTemporaryIndex()
{
$this->state = self::USE_TEMPORARY_INDEX;
}

/**
* Set the state to use regular Index
* @return void
*/
public function useRegularIndex()
{
$this->state = self::USE_REGULAR_INDEX;
}

/**
* @return string
*/
public function getState()
{
return $this->state;
}
}
43 changes: 43 additions & 0 deletions app/code/Magento/Inventory/Indexer/Scope/TemporaryResolver.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Inventory\Indexer\Scope;

use Magento\Framework\Indexer\ScopeResolver\IndexScopeResolver;
use Magento\Framework\Search\Request\Dimension;

/**
* Resolves name of a temporary table for indexation
* @todo refactoring it copy from catalog search module
*/
class TemporaryResolver implements \Magento\Framework\Search\Request\IndexScopeResolverInterface
Copy link

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

{
/**
* @var IndexScopeResolver
*/
private $indexScopeResolver;

/**
* @inheritDoc
*/
public function __construct(IndexScopeResolver $indexScopeResolver)
{
$this->indexScopeResolver = $indexScopeResolver;
}

/**
* @param string $index
* @param Dimension[] $dimensions
* @return string
*/
public function resolve($index, array $dimensions)
{
$tableName = $this->indexScopeResolver->resolve($index, $dimensions);
$tableName .= \Magento\Framework\Indexer\Table\StrategyInterface::TMP_SUFFIX;

return $tableName;
}
}
18 changes: 18 additions & 0 deletions app/code/Magento/Inventory/Indexer/Scope/UnknownStateException.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/

namespace Magento\Inventory\Indexer\Scope;

use Magento\Framework\Exception\LocalizedException;

/**
* Exception for situation where used state which is not defined in configuration
* @todo refactoring it copy from catalog search module
* @api
*/
class UnknownStateException extends LocalizedException
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

{
}
Loading