Improvements on the Reindex API #1752
Conversation
| $this->setParam(self::PIPELINE, $pipeline); | ||
| } | ||
|
|
||
| public function setRefresh($value): void |
There was a problem hiding this comment.
Should we force the value type to be bool $value?
There was a problem hiding this comment.
The valid values are: true, false and wait_for.
https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-reindex.html#docs-reindex-api-query-params
We could also create some class constants for them.
There was a problem hiding this comment.
I like your solution, it would be cleaner IMHO and would allow us to validate the parameter... or should it be something for the lower-level client library? 🤔
@ruflin , what do you think?
There was a problem hiding this comment.
In general I like the idea of validation at the same time it can put a burden on "us". Class constant would definitively be helpful but i would not validate that only these are used. If a new param is introduced in ES, we need to add it to support it which I don't think it optimal. Happy to also only have the most common / used ones as constants (or none to get started).
There was a problem hiding this comment.
I quite agree with the "burden", I just wanted to have a more strict typing here.
IIRC Elasticsearch accepts "true" (as string) and true (as boolean) as equals. if that's the case: we can allow setRefresh(string $value) and define 3 public constants as "true" "false" and "wait_for".
There was a problem hiding this comment.
I added some constants via fb3e933
There was a problem hiding this comment.
Thank you for the new fixes, what about only allowing string as parameter here?
renanbr
force-pushed
the
reindex-pipeline-refresh
branch
from
8133fd6
to
fb3e933
Compare
done via 06ffaa7 |
CHANGELOG.md
Outdated
| @@ -7,6 +7,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
| ## [Unreleased](https://github.com/ruflin/Elastica/compare/7.0.0-beta.3...master) | |||
| ### Backward Compatibility Breaks | |||
| ### Added | |||
| * Added `Elastica\Reindex->setPipeline(Elastica\Pipeline $pipeline): void`. The link between the reindex and the pipeline is solved when `run()` is called, and thus the pipeline given doesn't need to be created before calling `setPipeline()` | |||
There was a problem hiding this comment.
Nit: Could you add the links to the PR's? I'm just realising we miss it in some entries below too :-(
|
@ruflin LGTM, just one comment about the "setRefresh()" parameters |
|
I merged this PR to get it in. I see @renanbr 👍 your comment so I assume there will be a follow up PR? :-) |
yes, I'll do it |
Reindex improvements:
Reindex::PIPELINE$reindex->setPipeline(Pipeline $pipeline)$reindex->setRefresh(bool|string $value)$reindex->setQuery(AbstractQuery $query)Reindex::SLICESReindex::SLICES_AUTO... and also:
$pipeline->getId()