-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update ElasticSearch and Elastica to 6.x #1880
base: qa/2.x
Are you sure you want to change the base?
Conversation
9f05a24
to
1e9114f
Compare
1e9114f
to
e6a7473
Compare
7bdf937
to
0842eb9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking great @anvit!
I have not been able to look at everything in detail yet, but I have added a few comments that I think are important and may require some work already. Let me know your thoughts ;)
$itemType = QubitSearch::getInstance()->index->getIndexTypeName($item['type']); | ||
$search->addIndex($index)->addType($index->getType($itemType)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using a different type on each index, I'd suggest to use the dummy type name _doc
, as it will be used in 7.x:
https://www.elastic.co/blog/removal-of-mapping-types-elasticsearch#schedule
Using the same type for all indices should simplify many places modified in this PR.
$bulk->setType('QubitInformationObject'); | ||
$type = 'QubitInformationObject'; | ||
$bulk->setIndex(QubitSearch::getInstance()->index->getType($type)->getName()); | ||
$bulk->setType(QubitSearch::getInstance()->index->getIndexTypeName($type)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be just $bulk->setType("_doc");
{ | ||
$this->_instance = $instance; | ||
$this->_instance = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels strange to use an index decorator to manage multiple indexes. I'd remove this class and move the logic altogether to arElasticSearchPlugin.class.php
.
!isset($propertyProperties['copy_to']) | ||
|| ('_all' == $propertyProperties['copy_to']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This condition will now copy to _all
all the mapping fields where you removed include_in_all: false
. It may be better to add copy_to: _all
to the fields we really want to include and change this condition to isset($propertyProperties['copy_to']) && ('_all' == $propertyProperties['copy_to'])
.
Update ElasticSearch to 6.8.23 in the Dockerfile and update Elastica to 6.x in composer.
- 'inline' scripts are deprecated, changed to 'source'
Update arElasticSearchPlugin to use multiple indices instead of multiple types since ES 6.x removed being able to add multiple types. Also update mapping to remove include_in_all and add that to the _all field using copy_to instead since include_in_all was also removed in ES 6.x
Update autocompleteAction to use multiple indices
Add the appropriate index types for ES requests that did not need to specify explicit types in ES 5.x
Add missing document type to partialUpdate in arElasticSearchPlugin
Update condition in arElasticSearchPluginUtil to only get string fields included in _all if copy_to is explicitly set to _all in the mappping.
abb1768
to
0259ed0
Compare
Update ElasticSearch and Elastica and address issues introduced by breaking changes in ElasticSearch 6.x.
The most notable changes are:
include_in_all
mapping parameter is disallowed and the_all
field is disabled by default.Changes in Elastica: