-
Notifications
You must be signed in to change notification settings - Fork 43
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
BUGFIX: Version 2.x of elasticsearch does neither need nor accept index_name in fields mapping #59
Conversation
…x_name in fields mapping
@@ -57,6 +57,12 @@ class EntityMappingBuilder | |||
protected $indexInformer; | |||
|
|||
/** | |||
* @var int | |||
* @Flow\InjectConfiguration(path="driver.version") |
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 configuration option should be documented
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.
I've added the relevant part to the README.md
@@ -1,5 +1,6 @@ | |||
Flowpack: | |||
ElasticSearch: | |||
version: 2.x |
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.
Would the default version 1.x break most installations? Not sure if we should set 1.x or 2.x to default?! For sure this option is nowhere else used atm but could be in the future
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.
I think it would be better to set the default value to 1.x. So the package behaviour won't change out of the box and you have to intentionally change the setting to get a different result.
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.
For us this works well. We run our application on Neow/Flow 4.0.5 with Elasticsearch 5.4.1. We´re looking forward to find this request merged to receive it with the next release - good job, thanks.
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.
@patricweihmayer You run Elasticsearch 5.4.1? With which version of the CR adaptor? 😯
Based on the documentation elastic/elasticsearch#6677 index_name is deprecated in favor of copy_to, so maybe we can trigger an exception when the driver is switched to 2.x and we detect index_name, like this we can help user to edit the annotation and use copy_to ? What do you think ? In the current solution we silently ignore something that should not work. But to have a working situation we should have ... distinct annotations for 1.x and 2.x ... so hard to say:
|
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.
Silently ignoring should breaking existing project, basically if two fields use the same value for index_name, both field value are merged in a single elasticsearch field or if index_name is different from the model property name, this feature rename the field in elastic.
So if we ignore this field (and the value does not match the field name), the field will be empty ... so the index is broken.
For the ES v5 adjustments I decided to transparently map some things that have been changed compared to v2. Maybe in this case we could do the same, and for 2.x automatically transform |
Ah, but then again… I really like the driver pattern approach! |
What is the status here? |
AFAIK these issues have all been fixed and index_name is no longer supported, so closing. |
Using an annotation like this:
with version 2.x of elasticsearch throws the error: