Added Reindex and deprecated CrossIndex #1315
Conversation
lib/Elastica/Reindex.php
Outdated
| { | ||
| $body = self::getBody($oldIndex, $newIndex, $options); | ||
|
|
||
| $reindexEndpoint = new \Elasticsearch\Endpoints\Reindex(); |
There was a problem hiding this comment.
glad to see you used the endpoints 👍
lib/Elastica/Reindex.php
Outdated
| const SIZE = 'size'; | ||
| const QUERY = 'query'; | ||
|
|
||
| public static function reindex(Index $oldIndex, Index $newIndex, array $options = []) |
There was a problem hiding this comment.
Why did you use static method here? I'm thinking the implementation would become simpler if you would use a Reindex object where old, new and options are passed.
There was a problem hiding this comment.
I've used static method because there are not external depenendencies but maybe, to be compliant with OOP I should remove static. Do you suggest to pass parameters into constructor or in reindex method ?
There was a problem hiding this comment.
I was thinking passing params to constructor and then having something like $reindex->Run() or Start or something similar. Perhaps Start() could also take additional options. Not 100% sure yet how the most common use case will look like. Any thoughts?
There was a problem hiding this comment.
I think that call new and ->run() (or ->start()) are ever near to constructor and there is no need to change options after have called new Reindex(...). Maybe in a first time we can see how community uses reindex and if there will be need to change the use, we will change it consequently.
giovannialbero1992
force-pushed
the
deprecate_cross_index_and_add_reindex
branch
from
417d53e to
974b6e9
Compare
|
@ruflin I've removed static from class and renamed method in |
lib/Elastica/Reindex.php
Outdated
| /** | ||
| * @var Index | ||
| */ | ||
| private $oldIndex; |
There was a problem hiding this comment.
We always use $_oldIndex for all the variable names which are private / protected but perhaps the time has come to break this as not much other projects are left with the old pattern.
There was a problem hiding this comment.
I changed to be compliant with old pattern... maybe we can change the pattern into another commit on all protected/private variables :)
lib/Elastica/Reindex.php
Outdated
| /** | ||
| * @var Index | ||
| */ | ||
| private $newIndex; |
There was a problem hiding this comment.
Please make these all protected and not private. In case someone wants to extend Reindex in his code, he should be able to.
lib/Elastica/Reindex.php
Outdated
| return $this->newIndex; | ||
| } | ||
|
|
||
| private function getBody($oldIndex, $newIndex, $options) |
There was a problem hiding this comment.
Make these also protected, not private.
giovannialbero1992
force-pushed
the
deprecate_cross_index_and_add_reindex
branch
from
974b6e9 to
5906153
Compare
|
@ruflin, I've changed it :) |
giovannialbero1992
force-pushed
the
deprecate_cross_index_and_add_reindex
branch
from
5906153 to
497f298
Compare
lib/Elastica/Reindex.php
Outdated
| /** | ||
| * @var array | ||
| */ | ||
| private $_options; |
There was a problem hiding this comment.
This one should also be protected.
lib/Elastica/Reindex.php
Outdated
| return $this->_newIndex; | ||
| } | ||
|
|
||
| protected function getBody($oldIndex, $newIndex, $options) |
lib/Elastica/Reindex.php
Outdated
| ], $this->resolveDestOptions($options)); | ||
| } | ||
|
|
||
| private static function resolveSourceOptions(array $options) |
There was a problem hiding this comment.
This should also be not static and start with _* Also make it protected if there is no need to make it private.
giovannialbero1992
force-pushed
the
deprecate_cross_index_and_add_reindex
branch
from
497f298 to
eb49b81
Compare
|
@ruflin I've changed all names with '_' and removed a missing static from method. |
|
@giovannialbero1992 Thanks for your contribution. |
added changelog entry and asserts to config test added SpanOrTest added SpanMulti span functions in dsl query updated removed unnecessary attributes reverted docker ip range Changelog update Changelog update
#1311