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

[2.2] - Add command to view mview state and queue #12122

Merged
merged 18 commits into from
Dec 7, 2017
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
89 changes: 77 additions & 12 deletions app/code/Magento/Indexer/Console/Command/IndexerStatusCommand.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Magento\Framework\Indexer;
use Magento\Framework\Mview;

/**
* Command for displaying status of indexers.
Expand All @@ -30,21 +32,84 @@ protected function configure()
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
$table = $this->getHelperSet()->get('table');
$table->setHeaders(['Title', 'Status', 'Update On', 'Schedule Status', 'Schedule Updated']);

$rows = [];

$indexers = $this->getIndexers($input);
foreach ($indexers as $indexer) {
$status = 'unknown';
switch ($indexer->getStatus()) {
case \Magento\Framework\Indexer\StateInterface::STATUS_VALID:
$status = 'Ready';
break;
case \Magento\Framework\Indexer\StateInterface::STATUS_INVALID:
$status = 'Reindex required';
break;
case \Magento\Framework\Indexer\StateInterface::STATUS_WORKING:
$status = 'Processing';
break;
$view = $indexer->getView();

$rowData = [
'Title' => $indexer->getTitle(),
'Status' => $this->getStatus($indexer),
'Update On' => $indexer->isScheduled() ? 'Schedule' : 'Save',
'Schedule Status' => '',
'Updated' => '',
];

if ($indexer->isScheduled()) {
$state = $view->getState();
$rowData['Schedule Status'] = "{$state->getStatus()} ({$this->getPendingCount($view)} in backlog)";
$rowData['Updated'] = $state->getUpdated();
}
$output->writeln(sprintf('%-50s ', $indexer->getTitle() . ':') . $status);

$rows[] = $rowData;
}

usort($rows, function ($comp1, $comp2) {
return strcmp($comp1['Title'], $comp2['Title']);
});

$table->addRows($rows);
$table->render($output);
}

/**
* @param Indexer\IndexerInterface $indexer
* @return string
*/
protected function getStatus(Indexer\IndexerInterface $indexer)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you use private method visibility instead of protected?
Same for getPendingCount method

{
$status = 'unknown';
switch ($indexer->getStatus()) {
case \Magento\Framework\Indexer\StateInterface::STATUS_VALID:
$status = 'Ready';
break;
case \Magento\Framework\Indexer\StateInterface::STATUS_INVALID:
$status = 'Reindex required';
break;
case \Magento\Framework\Indexer\StateInterface::STATUS_WORKING:
$status = 'Processing';
break;
}
return $status;
}

/**
* @param Mview\ViewInterface $view
* @return string
*/
protected function getPendingCount(Mview\ViewInterface $view)
{
$changelog = $view->getChangelog();

try {
$currentVersionId = $changelog->getVersion();
} catch (Mview\View\ChangelogTableNotExistsException $e) {
return '';
}

$state = $view->getState();

$pendingCount = $changelog->getListSize($state->getVersionId(), $currentVersionId);

Choose a reason for hiding this comment

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

\Magento\Framework\Mview\ViewInterface::getChangelog returns \Magento\Framework\Mview\View\ChangelogInterface so method getListSize cannot be used here as it is declared in ChangelogCounterInterface and is unkown in this code.

Seems there is no clean and backward compatible solution to implement counting without list load. I propose in the scope of this PR implement counting as was done in the initial version with count($changelog->getList()) and process this PR to deliver this feature to M2. Once PR will be merged new PR may be created with a refactoring of changelog and performance optimization.

@convenient, @ihor-sviziev, @kandy are you agree with this plan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @vkublytskyi I was really struggling to balance out clean/backwards-compatability as you noted.

If @kandy agrees my plan of action will be

  1. Roll back to count($changelog->getList()) with associated tests
  2. Update description/title to properly highlight what is occurring in this PR
  3. Wait for approval from all parties (or potentially for merge?)
  4. Backport changes to [2.1] - Add command to view mview state and queue #12050

Sound good?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, looks good for me


$pendingString = "<error>$pendingCount</error>";
if ($pendingCount <= 0) {
$pendingString = "<info>$pendingCount</info>";
}

return $pendingString;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
use Magento\Framework\Indexer\StateInterface;
use Magento\Indexer\Console\Command\IndexerStatusCommand;
use Symfony\Component\Console\Tester\CommandTester;
use Symfony\Component\Console\Helper\HelperSet;
use Symfony\Component\Console\Helper\TableHelper;

class IndexerStatusCommandTest extends AbstractIndexerCommandCommonSetup
{
Expand All @@ -18,35 +20,132 @@ class IndexerStatusCommandTest extends AbstractIndexerCommandCommonSetup
*/
private $command;

/**
* @param \PHPUnit_Framework_MockObject_MockObject $indexerMock
* @param array $data
* @return mixed
*/
protected function attachViewToIndexerMock($indexerMock, array $data)
{
/** @var \Magento\Framework\Mview\View\Changelog|\PHPUnit_Framework_MockObject_MockObject $stub */
$changelog = $this->getMockBuilder(\Magento\Framework\Mview\View\Changelog::class)
->disableOriginalConstructor()
->getMock();

$changelog->expects($this->any())
->method('getListSize')
->willReturn($data['view']['changelog']['list_size']);

/** @var \Magento\Indexer\Model\Mview\View\State|\PHPUnit_Framework_MockObject_MockObject $stateMock */
$stateMock = $this->getMockBuilder(\Magento\Indexer\Model\Mview\View\State::class)
->disableOriginalConstructor()
->setMethods(null)
->getMock();

$stateMock->addData($data['view']['state']);

/** @var \Magento\Framework\Mview\View|\PHPUnit_Framework_MockObject_MockObject $viewMock */
$viewMock = $this->getMockBuilder(\Magento\Framework\Mview\View::class)
->disableOriginalConstructor()
->setMethods(['getChangelog', 'getState'])
->getMock();

$viewMock->expects($this->any())
->method('getState')
->willReturn($stateMock);
$viewMock->expects($this->any())
->method('getChangelog')
->willReturn($changelog);

$indexerMock->method('getView')
->willReturn($viewMock);

return $indexerMock;
}

/**
* @param array $indexers
* @param array $statuses
*
* @dataProvider executeAllDataProvider
*/
public function testExecuteAll(array $indexers, array $statuses)
public function testExecuteAll(array $indexers)
{
$this->configureAdminArea();
$indexerMocks = [];
foreach ($indexers as $indexerData) {
$indexerMock = $this->getIndexerMock(
['getStatus'],
['getStatus', 'isScheduled', 'getState', 'getView'],
$indexerData
);

$indexerMock->method('getStatus')
->willReturn($statuses[$indexerData['indexer_id']]);
->willReturn($indexerData['status']);
$indexerMock->method('isScheduled')
->willReturn($indexerData['is_scheduled']);

if ($indexerData['is_scheduled']) {
$this->attachViewToIndexerMock($indexerMock, $indexerData);
}

$indexerMocks[] = $indexerMock;

}
$this->initIndexerCollectionByItems($indexerMocks);
$this->command = new IndexerStatusCommand($this->objectManagerFactory);

$objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);

$this->command->setHelperSet(
$objectManager->getObject(
HelperSet::class,
['helpers' => [$objectManager->getObject(TableHelper::class)]]
)
);


$commandTester = new CommandTester($this->command);
$commandTester->execute([]);
$actualValue = $commandTester->getDisplay();
$expectedValue = sprintf('%-50s ', 'Title_indexerOne' . ':') . 'Ready' . PHP_EOL
. sprintf('%-50s ', 'Title_indexerTwo' . ':') . 'Reindex required' . PHP_EOL
. sprintf('%-50s ', 'Title_indexerThree' . ':') . 'Processing' . PHP_EOL
. sprintf('%-50s ', 'Title_indexerFour' . ':') . 'unknown' . PHP_EOL;

$this->assertStringStartsWith($expectedValue, $actualValue);
$linesOutput = array_filter(explode(PHP_EOL, $commandTester->getDisplay()));

$this->assertCount(8, $linesOutput, 'There should be 8 lines output. 3 Spacers, 1 header, 4 content.');
$this->assertEquals($linesOutput[0], $linesOutput[2], "Lines 0, 2, 7 should be spacer lines");
$this->assertEquals($linesOutput[2], $linesOutput[7], "Lines 0, 2, 6 should be spacer lines");
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably there is mistake


$headerValues = array_values(array_filter(explode('|', $linesOutput[1])));
$this->assertEquals('Title', trim($headerValues[0]));
$this->assertEquals('Status', trim($headerValues[1]));
$this->assertEquals('Update On', trim($headerValues[2]));
$this->assertEquals('Schedule Status', trim($headerValues[3]));
$this->assertEquals('Schedule Updated', trim($headerValues[4]));

$indexer1 = array_values(array_filter(explode('|', $linesOutput[3])));
$this->assertEquals('Title_indexer1', trim($indexer1[0]));
$this->assertEquals('Ready', trim($indexer1[1]));
$this->assertEquals('Schedule', trim($indexer1[2]));
$this->assertEquals('idle (10 in backlog)', trim($indexer1[3]));
$this->assertEquals('2017-01-01 11:11:11', trim($indexer1[4]));

$indexer2 = array_values(array_filter(explode('|', $linesOutput[4])));
$this->assertEquals('Title_indexer2', trim($indexer2[0]));
$this->assertEquals('Reindex required', trim($indexer2[1]));
$this->assertEquals('Save', trim($indexer2[2]));
$this->assertEquals('', trim($indexer2[3]));
$this->assertEquals('', trim($indexer2[4]));

$indexer3 = array_values(array_filter(explode('|', $linesOutput[5])));
$this->assertEquals('Title_indexer3', trim($indexer3[0]));
$this->assertEquals('Processing', trim($indexer3[1]));
$this->assertEquals('Schedule', trim($indexer3[2]));
$this->assertEquals('idle (100 in backlog)', trim($indexer3[3]));
$this->assertEquals('2017-01-01 11:11:11', trim($indexer3[4]));

$indexer4 = array_values(array_filter(explode('|', $linesOutput[6])));
$this->assertEquals('Title_indexer4', trim($indexer4[0]));
$this->assertEquals('unknown', trim($indexer4[1]));
$this->assertEquals('Schedule', trim($indexer4[2]));
$this->assertEquals('running (20 in backlog)', trim($indexer4[3]));
$this->assertEquals('2017-01-01 11:11:11', trim($indexer4[4]));
}

/**
Expand All @@ -59,27 +158,65 @@ public function executeAllDataProvider()
'indexers' => [
'indexer_1' => [
'indexer_id' => 'indexer_1',
'title' => 'Title_indexerOne'
'title' => 'Title_indexer1',
'status' => StateInterface::STATUS_VALID,
'is_scheduled' => true,
'view' => [
'state' => [
'status' => 'idle',
'updated' => '2017-01-01 11:11:11',
],
'changelog' => [
'list_size' => 10
]
]
],
'indexer_2' => [
'indexer_id' => 'indexer_2',
'title' => 'Title_indexerTwo'
'title' => 'Title_indexer2',
'status' => StateInterface::STATUS_INVALID,
'is_scheduled' => false,
'view' => [
'state' => [
'status' => 'idle',
'updated' => '2017-01-01 11:11:11',
],
'changelog' => [
'list_size' => 99999999
]
]
],
'indexer_3' => [
'indexer_id' => 'indexer_3',
'title' => 'Title_indexerThree'
'title' => 'Title_indexer3',
'status' => StateInterface::STATUS_WORKING,
'is_scheduled' => true,
'view' => [
'state' => [
'status' => 'idle',
'updated' => '2017-01-01 11:11:11',
],
'changelog' => [
'list_size' => 100
]
]
],
'indexer_4' => [
'indexer_id' => 'indexer_4',
'title' => 'Title_indexerFour'
'title' => 'Title_indexer4',
'status' => null,
'is_scheduled' => true,
'view' => [
'state' => [
'status' => 'running',
'updated' => '2017-01-01 11:11:11',
],
'changelog' => [
'list_size' => 20
]
]
],
],
'Statuses' => [
'indexer_1' => StateInterface::STATUS_VALID,
'indexer_2' => StateInterface::STATUS_INVALID,
'indexer_3' => StateInterface::STATUS_WORKING,
'indexer_4' => null,
]
],
];
}
Expand Down
36 changes: 32 additions & 4 deletions lib/internal/Magento/Framework/Mview/View/Changelog.php
Original file line number Diff line number Diff line change
Expand Up @@ -127,14 +127,12 @@ public function clear($versionId)
}

/**
* Retrieve entity ids by range [$fromVersionId..$toVersionId]
*
* @param int $fromVersionId
* @param int $toVersionId
* @return int[]
* @return \Magento\Framework\DB\Select
* @throws ChangelogTableNotExistsException
*/
public function getList($fromVersionId, $toVersionId)
protected function getListSelect($fromVersionId, $toVersionId)
Copy link
Contributor

Choose a reason for hiding this comment

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

All new added methods should have private or public visibility

{
$changelogTableName = $this->resource->getTableName($this->getName());
if (!$this->connection->isTableExists($changelogTableName)) {
Expand All @@ -154,9 +152,39 @@ public function getList($fromVersionId, $toVersionId)
(int)$toVersionId
);

return $select;
}

/**
* Retrieve entity ids by range [$fromVersionId..$toVersionId]
*
* @param int $fromVersionId
* @param int $toVersionId
* @return int[]
* @throws ChangelogTableNotExistsException
*/
public function getList($fromVersionId, $toVersionId)
{
$select = $this->getListSelect($fromVersionId, $toVersionId);
return $this->connection->fetchCol($select);
}

/**
* Retrieve the count of entity ids in the range [$fromVersionId..$toVersionId]
*
* @param int $fromVersionId
* @param int $toVersionId
* @return int[]
* @throws ChangelogTableNotExistsException
*/
public function getListSize($fromVersionId, $toVersionId)
Copy link
Contributor

@ihor-sviziev ihor-sviziev Dec 3, 2017

Choose a reason for hiding this comment

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

This method was just added, this change is not backward compatible. Unfortunately we can't accept such change. Couldn't it be done without adding new methods?
If no - then we need to introduce new interface that will include new method.

Copy link
Contributor Author

@convenient convenient Dec 3, 2017

Choose a reason for hiding this comment

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

Thanks I was waiting for the suite to go green before raising questions about this function, this was my attempt to address this comment: #12122 (comment)

I don't believe this can be achieved without a similar query, I was trying to prevent the SELECT query from being generated in two places, but if the interface is not up for modification I can either do it in a new interface or directly within the Command class itself. How do you think this should be architected? If you'd suggest a new interface how do you think it should be named etc?

I'm very much appreciating all your feedback on this PR by the way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this 4af04b1 what you meant?

{
$countSelect = $this->getListSelect($fromVersionId, $toVersionId);
$countSelect->reset(\Magento\Framework\DB\Select::COLUMNS);
$countSelect->columns(new \Zend_Db_Expr(("COUNT(DISTINCT " . $this->getColumnName() . ")")));
return $this->connection->fetchOne($countSelect);
}

/**
* Get maximum version_id from changelog
* @return int
Expand Down
Loading