Add atomic reindex support, via --atomic flag in search:import

[ostrolucky]

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Related Issue	Fix #292
Need Doc update	yes

0 downtime search:clear && search:import solution

[ostrolucky]

Scout-extended is doing here try {} catch {}, but command does handle situation with missing original index fine according my testing.

[ostrolucky]

Nothing I can do with test failure

[chloelbn]

Hey @ostrolucky! Thanks for your PR. As we are getting ready for the release and this feature can be delivered as a minor, I'll postpone the review a little bit. Thanks for your patience!

[ostrolucky]

Rebased on master. 4.0 is out for some time. Any chance to take a look at this now?

[chloelbn]

Hi @ostrolucky , thank you for your patience! Overall it looks good but could you tell me why you favored a --clear option over a whole new reimport command like in Scout Extended?

[ostrolucky]

Two reasons

1. Code reuse. Alternative would be to duplicate lot of same code in both commands and you would have to keep changing both places each time you update one of them, or come up with elaborate way to extract shared logic to separate classes
2. Semantics. search:reimport name doesn't tell people anything about its difference from search:import. Quite contrary, it makes people falsely assume search:import can be used only when index is empty. And it still doesn't say search:reimport is idempotent, nor that it gets rid of data in index not available in database anymore. It's also easier for users to learn new flag of search:import rather than whole new command, which does almost same thing. It's IMO more intuitive to do all your import business through one command.

[chloelbn]

@ostrolucky I understand your explanation. In that case I'd rather change the flag for --atomic

[ostrolucky]

It doesn't convey that data is imported from scratch and obsolete data will be erased. How about --replace?

[chloelbn]

@ostrolucky it's the search:import command that does this, and your feature adds the atomic dimension to it. This is why I think the --atomic flag is the best. WDYT?
Moreover the fact that the tests fail on the Aggregator import test is not normal. This has to do with some entities in the aggregator being not searchable per se. Could you dig into it please?

[ostrolucky]

Indeed, failure with testSearchImportAggregator was valid. Fixed and renamed clear to atomic.

[nunomaduro]

I am a fan if your implementation, I like it.

One very important thing tho, we should allow the user to pass a --safe, that should call the wait method on the copy, on the sum of all responses ( after the while ), and on the very last move: https://github.com/algolia/algoliasearch-client-php/blob/ffa03964a56adafbb3bef5b06d7445ff2a73eb5b/src/SearchIndex.php#L217.

[ostrolucky]

One very important thing tho, we should allow the user to pass a --safe, that should call the wait method on the copy, on the sum of all responses ( after the while ), and on the very last move

Is it really useful? Algolia queues up these operations. IMHO it's useful only when having following operation retrieving data from algolia, which we don't do here. See from https://www.algolia.com/doc/api-client/methods/indexing/#asynchronous-methods: "What you are actually doing when calling these methods is adding a new job to a queue". My testing confirms that. Can you think of some scenario to reproduce failing of this command not using wait methods?

[nunomaduro]

@ostrolucky

Is it really useful?

Yes, but let's implement it later in another pull request ( doesn't have to be done by you! ).

Thanks for all the work so far.

[ostrolucky]

Alright, I think I addressed everything :)

[chloelbn]

Would you mind rebasing on master please?

[ostrolucky]

done