Allow skipping non Algolia entities in commands #109

gruberro · 2017-03-26T20:37:32Z

When invoking the clear or reindex command for all known entities the command will fail if at least one entity is not Algolia mapped. This is inkonsistent regarding the settings command and makes it
difficult in automation processes.

By adding a new optional command line settings this kind of error can be skipped. By default the command behavior is not changing (BC).

julienbourdeau · 2017-04-04T08:13:21Z

Hi @gruberro,

Thanks for your contribution to the project!

You're right the behavior seems a bit odd. I'm wondering if we really need an option for that.
Maybe by default, all entities that don't use any @Algolia\Attribute should be skipped.

I assume that a fairly large application, not all entities get indexed anyway.

What do you think?

cc @maxiloc @JanPetr @rayrutjes

gruberro · 2017-04-04T08:28:23Z

My intention to add it by option was to have the change backward compatible. Otherwise the default behavior would change. But to be honest having the commands skipping all non Algolia entities by default is somehow expected 😄 .

Your assumption is correct. In our applications only the business relevant entities are indexed!

julienbourdeau · 2017-04-04T09:08:19Z

I understand why you want to keep backward compat', it's very important and we should keep it as much as possible. Thanks for caring.

I'm trying to think about the drawbacks of modifying this behavior.

Today, if you have a non-Algolia entity you will get an exception, so all entities that was supposed to be indexed after are skipped.
I assume most people probably either have only algolia entities or worked around it (maybe by launching the command for each entity).

I feel like it adds some complexity and I can't think about a situation where you want/need an exception.

I came up with this poc to carry on the discussion: 7e72a38
By adding a message, we can still let people know about it. The internal behavior is not altered at all, only the Symfony command.

gruberro · 2017-04-04T11:35:47Z

You're absolutely right, I can't think of another situation you've not listed yet, so it might be safe to make skipping default as well. Can change this if you're ok with it!

maxiloc · 2017-04-05T09:48:25Z

Agree with @julienbourdeau I don't see any issue skipping them as long as you put a clear message that it was skipped

julienbourdeau · 2017-04-05T12:31:47Z

Hi @gruberro,

So everyone agrees! Can you rework your pull request? Then I'll be happy to merge it!

gruberro · 2017-04-05T20:55:46Z

@julienbourdeau did the small refactoring as we discussed before!

julienbourdeau

Thanks again for your contribution! I left a few comments on your PR.

I cannot get the test to pass, can you have a look please?

julienbourdeau · 2017-04-11T09:32:35Z

Command/ReindexCommand.php

    {
        $reIndexer = $this->getContainer()->get('algolia.indexer')->getManualIndexer($this->getObjectManager());

-        return $reIndexer->reIndex($className, [


Why did you remove the return here?

julienbourdeau · 2017-04-11T09:33:47Z

Command/ReindexCommand.php


        $nIndexed = 0;
        foreach ($toReindex as $className) {
-            $nIndexed += $this->reIndex($className, $batchSize, $safe);


Are you sure $this->reIndex() always returned 1? 🤔

Ah damn... is fixed!

julienbourdeau · 2017-04-11T09:34:49Z

Tests/AlgoliaSearch/ClearCommandTest.php

+
+    public function testClearWillFailWithNonAlgoliaMappedEntity()
+    {
+        $this->setExpectedException(


Please use expectException() since this method has been removed in PHPUnit 6.

gruberro · 2017-04-11T22:14:51Z

@julienbourdeau took a look and fixed the failing tests cases... at least it's stable with my local setup

julienbourdeau · 2017-04-12T15:55:17Z

Command/ClearCommand.php

+        foreach ($toClear as $className) {
+            try {
+                $this->clear($className);
+                $nCleared++;


Same here, I'm not sure about this. I'd rather keep the return statement in clear() and add the result to $nCleared.

In this particular case this won't make sense IMO as the underlaying indexer is not returning the number of affected documents (void) (https://github.com/algolia/AlgoliaSearchBundle/blob/master/Indexer/ManualIndexer.php#L166-L179). So counting the number of entity types being cleared is the only information that can be gathered here.

Indeed, Thanks!

julienbourdeau · 2017-04-12T16:03:28Z

Hi @gruberro,

Thanks for you edits. Can you have a look at my latest comment?

Also, it seems like you merge master into your branch. Would you mind cleaning your branch? If you're not very familiar with this I would recommend you to create a new one, cherry-pick your commit and push it to the same remote branch.

git checkout master
git pull upstream master
git checkout -b new-name
git cherry-pick d3e5fd2
git push -f origin new-name:allow-skipping-non-algolia-entities

Make sure that your git configuration is set to rebase on pull and not merge.
If you are interested you can read this article I wrote at my previous company: http://build.prestashop.com/howtos/misc/set-up-your-git-for-contributing/

Thanks again for your contribution!

gruberro · 2017-04-12T16:21:20Z

@julienbourdeau oh damn... seems it was way too late yesterday night. I'll rebase the PR.. sry 😟

julienbourdeau · 2017-04-14T12:21:56Z

Tests/AlgoliaSearch/ReindexCommandTest.php

        $input = new ArrayInput($options);

-        $this->command->run($input, new ConsoleOutput());
+        $this->command->run($input, new NullOutput());


Travis fails and I get the same result locally. I manage to get the tests passing but using ConsoleOutput like it was before.
Why did you change this to NullOutput?

After that I believe we'll be ready to merge! 🎉

I've changed it to NullOutput to avoid output to the console during test execution which may cause risky tests (at least if configured). But if it's breaking the build I'll change for sure (unfortunately it's not breaking the build on my machine 😟)! WDYT of using the BufferedOutput which could then also be used to check if some excepted output has been written?

I understand, I wish we could get rid of the "30 entities indexed" too. I tried on my local environnement, it didn't work so I expect problems with Travis as well.

Feel free to open a new pull request dedicated to that :)

Reverted my changes to ConsoleOutput as suggested by you. Will try to investigate your errors in a new pull request. Are your local errors the same as the one's on Travis? (No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.) Just to be sure to search for the same thingies 😄.

I don't think it's the same, I'm using Homestead and I think pretty much with everything by default.

When invoking the clear or reindex command for all known entities the command will fail if at least one entity is not Algolia mapped. This is incosistent regarding the settings command and makes it difficult in automation processes. With this change the commands will no longer trigger an error but skip non Algolia entities. An informational output will be shown.

demagu · 2017-06-08T19:38:11Z

@julienbourdeau can we get this merged please?

julienbourdeau · 2017-06-12T07:27:43Z

Yes, definitely. I'm looking into travis right now.

alcaeus · 2017-09-28T08:03:16Z

@julienbourdeau any luck with travis?

gruberro · 2017-11-30T20:59:38Z

@julienbourdeau really looking forward to 3.0, but is there any chance to bring this into 2.x?

julienbourdeau · 2017-12-01T10:05:05Z

Yes, I'll release a new version of 2.0 with most of the open PR. They make a lot of sense and I don't want to waste the work done here 👍

gruberro mentioned this pull request Mar 26, 2017

Allow skipping non Algolia entities in commands #55

Closed

julienbourdeau self-assigned this Apr 5, 2017

gruberro force-pushed the allow-skipping-non-algolia-entities branch 2 times, most recently from 50d2872 to 202738d Compare April 5, 2017 20:51

julienbourdeau suggested changes Apr 11, 2017

View reviewed changes

gruberro force-pushed the allow-skipping-non-algolia-entities branch from 202738d to d3e5fd2 Compare April 11, 2017 22:12

julienbourdeau reviewed Apr 12, 2017

View reviewed changes

gruberro force-pushed the allow-skipping-non-algolia-entities branch 5 times, most recently from a7a7b26 to dcd16f4 Compare April 12, 2017 16:39

julienbourdeau suggested changes Apr 14, 2017

View reviewed changes

gruberro force-pushed the allow-skipping-non-algolia-entities branch from dcd16f4 to 8de0880 Compare April 18, 2017 18:56

julienbourdeau approved these changes Apr 19, 2017

View reviewed changes

julienbourdeau changed the base branch from master to 2.x November 29, 2017 15:28

julienbourdeau added the v2 label Dec 4, 2017

julienbourdeau removed the v2 label Mar 5, 2018

julienbourdeau merged commit e023d9d into algolia:2.x Aug 28, 2018

Allow skipping non Algolia entities in commands #109

Allow skipping non Algolia entities in commands #109

Uh oh!

Conversation

gruberro commented Mar 26, 2017

Uh oh!

julienbourdeau commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gruberro commented Apr 4, 2017

Uh oh!

julienbourdeau commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gruberro commented Apr 4, 2017

Uh oh!

maxiloc commented Apr 5, 2017

Uh oh!

julienbourdeau commented Apr 5, 2017

Uh oh!

gruberro commented Apr 5, 2017

Uh oh!

julienbourdeau 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!

gruberro commented Apr 11, 2017

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!

julienbourdeau commented Apr 12, 2017

Uh oh!

gruberro commented Apr 12, 2017

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!

demagu commented Jun 8, 2017

Uh oh!

julienbourdeau commented Jun 12, 2017

Uh oh!

alcaeus commented Sep 28, 2017

Uh oh!

gruberro commented Nov 30, 2017

Uh oh!

julienbourdeau commented Dec 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

julienbourdeau commented Apr 4, 2017 •

edited

Loading

julienbourdeau commented Apr 4, 2017 •

edited

Loading