[2.2] - Add command to view mview state and queue #12122
Conversation
This is similar to the magerun1 command here: netz98/n98-magerun#891 I like the ability to view the mview queue in realtime as its being processed, it can be quite helpful when debugging indexing issues. This command will actually show how many items are in the list pending processing, as well information from the `mview_state` table. ``` php bin/magento indexer:status:mview +---------------------------+----------+--------+---------------------+------------+---------+ | ID | Mode | Status | Updated | Version ID | Backlog | +---------------------------+----------+--------+---------------------+------------+---------+ | catalog_category_product | enabled | idle | 2017-11-02 10:00:00 | 1 | 0 | | catalog_product_attribute | enabled | idle | 2017-11-02 10:00:00 | 1 | 1 | | catalog_product_category | disabled | idle | 2017-11-02 10:00:00 | 1 | 0 | | catalog_product_price | enabled | idle | 2017-11-02 10:00:00 | 1 | 0 | +---------------------------+----------+--------+---------------------+------------+---------+ ``` I'll point this PR into 2.1.x and raise a separate PR to pop it into 2.2.x.
convenient
changed the title
|
I personally more like the idea to update indexer:status command instead of introducing of new. |
| continue; | ||
| } | ||
|
|
||
| $pendingCount = count($changelog->getList($state->getVersionId(), $currentVersionId)); |
There was a problem hiding this comment.
I believe that this code will have memory usage issue in case of huge number of item in log
There was a problem hiding this comment.
Good point, it's defined as
public function getList($fromVersionId, $toVersionId)
{
$changelogTableName = $this->resource->getTableName($this->getName());
if (!$this->connection->isTableExists($changelogTableName)) {
throw new ChangelogTableNotExistsException(new Phrase("Table %1 does not exist", [$changelogTableName]));
}
$select = $this->connection->select()->distinct(
true
)->from(
$changelogTableName,
[$this->getColumnName()]
)->where(
'version_id > ?',
(int)$fromVersionId
)->where(
'version_id <= ?',
(int)$toVersionId
);
return $this->connection->fetchCol($select);
}I'll address.
|
@convenient Please let us know when it's updated. |
|
@ishakhsuvarov can i have some clarification please. Is this comment a requirement or a suggestion?
I'd prefer to keep them separate tbh, but let me know please. |
|
@convenient from my perspective would be great to improve existing indexer:status command because mview status it's like part of indexer status (see as highly recommended), but if you can't do it right now - probably we could accept your implementation as MVP. |
|
I've given this a quick hack locally and I can't find a pretty way of outputting all the data I'd need to vary between the scheduled indexers, and the non scheduled indexers. Having the Either way I'm happy to do whatever is required, I just want to know how it should look at the end. Bundling both the scheduled and the non-scheduled indexers into the same table as above really makes the What does everyone think? |
|
@convenient probably show all information that we have isn't really good idea. I would like to remove "version id" column, and data from "backlog" column show as part of "schedule status", like "Idle (4 in backlog)", I think it will be better in this way. |
It looks a little neater, but i'm usually a opposed to bundling semantic data into a string like this which would be a bit harder to This data would still make |
|
@convenient from my perspective - looks good. @ishakhsuvarov could you review latest comment. Would it be ok to improve this command in this way? |
|
Once we've decided on how the output looks i'll update the test suite and push up my commits, just delaying doing any further work until the output is agreed. |
|
@convenient I had a conversation with @kandy and he is ok with the proposed output.
It still may be usable if someone need only to check when indexer will be triggered. Plus removing of CLI commands are not allowed due to backward compatibility policy. |
|
@vkublytskyi thanks, i'm working on this and will update soon. |
| * @param Indexer\IndexerInterface $indexer | ||
| * @return string | ||
| */ | ||
| protected function getStatus(Indexer\IndexerInterface $indexer) |
There was a problem hiding this comment.
Could you use private method visibility instead of protected?
Same for getPendingCount method
|
|
||
| $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"); |
There was a problem hiding this comment.
Probably there is mistake
| * @throws ChangelogTableNotExistsException | ||
| */ | ||
| public function getList($fromVersionId, $toVersionId) | ||
| protected function getListSelect($fromVersionId, $toVersionId) |
There was a problem hiding this comment.
All new added methods should have private or public visibility
| * @return int[] | ||
| * @throws ChangelogTableNotExistsException | ||
| */ | ||
| public function getListSize($fromVersionId, $toVersionId) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is this 4af04b1 what you meant?
Use private method visibility instead of protected in test
|
|
||
| $state = $view->getState(); | ||
|
|
||
| $pendingCount = $changelog->getListSize($state->getVersionId(), $currentVersionId); |
There was a problem hiding this comment.
\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?
There was a problem hiding this comment.
Thanks @vkublytskyi I was really struggling to balance out clean/backwards-compatability as you noted.
If @kandy agrees my plan of action will be
- Roll back to
count($changelog->getList())with associated tests - Update description/title to properly highlight what is occurring in this PR
- Wait for approval from all parties (or potentially for merge?)
- Backport changes to [2.1] - Add command to view mview state and queue #12050
Sound good?
|
Sure
|
This is not so performant, but as discussed in github it'll do for now.
[EngCom] Public Pull Requests - 2.2-develop - MAGETWO-84981: Trying to get data from non existent products #12539 - MAGETWO-84979: [Backport 2.2-develop] Fix swagger-ui on instances of Magento running on a non-standard port #12541 - MAGETWO-84903: Added namespace to product videos fotorama events #12469 - MAGETWO-84862: [Backport 2.2-develop] #11409: Too many password reset requests even when disabled in settings #11435 - MAGETWO-84856: Issue 12506: Fixup typo getDispretionPath -> getDispersionPath #12507 - MAGETWO-84808: 12110: Missing cascade into attribute set deletion. #12167 - MAGETWO-83503: [2.2] - Add command to view mview state and queue #12122 - MAGETWO-80223: Fix syntax of expectException() calls #11099
Description
Update
indexer:statusto output information about the schedule mview backlogManual testing scenarios
php bin/magento indexer:set-mode schedule catalog_product_price_cltablephp bin/magento indexer:statusto see the backlog increasemagerun2 sys:cron:run indexer_update_all_viewswill trigger the mview processesOriginal DescriptionThis is similar to the magerun1 command here: netz98/n98-magerun#891I like the ability to view the mview queue in realtime as its being processed, it can be quite helpful when debugging indexing issues.This command will actually show how many items are in the list pending processing, as well information from themview_statetable.Contribution checklist