- ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
Just coming back to this, we can't change or remove field mappings on existing indexes, we can only add additional field mappings.
- ๐ฌ๐งUnited Kingdom longwave UK
Also running into this, even just a minor change to an index config that could simply be reindexed over the existing content means the index is cleared during deployment.
Index::postSave()
already has some logic to handle this problem, can we just rely on this? Or at least detect the situation where fields are changed/removed and only clear in that case?// React on possible changes that would require re-indexing, etc. $this->reactToServerSwitch($original); $this->reactToDatasourceSwitch($original); $this->reactToTrackerSwitch($original); $this->reactToProcessorChanges($original);
- ๐ฌ๐งUnited Kingdom longwave UK
A method we have tried to use to work around this is blue/green deployments; we have two identical indexes, and when we need to make a change we write to index 2 before switching reads to use index 2, and then swap over again next time around. It would be nice if this could be supported natively but not sure if this would be better off in Search API itself or in here.
- First commit to issue fork.
- Merge request !41Issue #3285438: Prevent index clear when field mapping hasn't changed. โ (Closed) created by robphillips
34:57 29:14 Running38:32 29:14 Running- Status changed to Needs review
10 months ago 3:22am 8 February 2024 32:03 29:14 Running- ๐บ๐ธUnited States robphillips
Took a crack at the problem. Search API Solr offers up a similar solution. Re-index is only triggered when a indexed field has changed. Tested with both in-site configuration changes and using configuration sync. There might be some edge cases. Definitely needs more eyes on it.
https://git.drupalcode.org/project/search_api_solr/-/blob/4.x/src/Plugin...
The last submitted patch, 6: 3285438-5.patch, failed testing. View results โ
- ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
Hiding the patch because we are using MRs and Gitlab CI now.
- achap ๐ฆ๐บ
Thanks for the MR. Checking if fields have changed does seem like a good idea but I think a clear rather than a re-index will be necessary as alluded to in previous comments. From the Opensearch docs:
If you want to create or add mappings and fields to an index, you can use the put mapping API operation. For an existing mapping, this operation updates the mapping.
You canโt use this operation to update mappings that already map to existing data in the index. You must first create a new index with your desired mappings, and then use the reindex API operation to map all the documents from your old index to the new index. If you donโt want any downtime while you re-index your indexes, you can use aliases.
See docs: https://opensearch.org/docs/latest/api-reference/index-apis/put-mapping/
So if we could detect if a field doesn't yet exist then we could use the re-index flag, and if it does already exist then we would need to clear. However that could get complicated if one field is added and another changed etc.
So just checking if any fields exist in new index vs original and only clearing then already seems like an improvement. However I think the clear would need to come before the call to updateSettings and updateFieldMapping
- Status changed to Needs review
5 months ago 5:06am 13 June 2024 - achap ๐ฆ๐บ
I took a stab at this with MR#52
It retrieves the mappings from the OpenSearch server and compares them with the local Drupal mappings after event subscribers have fired.
I don't think comparing the original index fields to the new ones will work because the modified mappings after event subscribers have fired aren't stored anywhere and so it's not really possible to compare.
The other thing to be aware of is the dynamic mappings in OpenSearch. If you don't explicitly provide a type it will guess based off the data that is indexed. That means that every time you click save even if nothing has changed it will clear the index (because there are no local mappings). This is mostly a problem if you create fields in processors etc and the fix is just to use the IndexParamsEvent to explicitly give your custom fields the correct types. I have found that if you incorrectly give a Drupal field that is a float for example a string type in Search API then OS will override it with the float type in the mappings once data has been indexed. That will also cause clearing rather than re-indexing. So you need to be careful to assign the correct type.
Did not tackle settings (analysers etc), only field mappings and doesn't negate the need for a blue/green deployment but should definitely help reduce the amount of times you need to switch indexes.
- ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
This MR will need to be rebased to go onto 3.x branch. It's currently on 1.x which is security fixes only.
- Status changed to Needs work
5 months ago 4:36am 14 June 2024 - ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
NW for feedback and test fails
- Status changed to Needs review
5 months ago 5:32am 14 June 2024 - achap ๐ฆ๐บ
Have updated the code based on feedback. Looks like that error was unrelated to my code but I was able to trace it to the
search_api_item
table not being installed in the SpellCheckTest so I added there. - Status changed to RTBC
5 months ago 5:37am 14 June 2024 - ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
This looks good to me. I'll leave it as RTBC in case anyone else has feedback over the next 24hrs.
- Status changed to Needs review
5 months ago 6:22am 14 June 2024 - achap ๐ฆ๐บ
Setting back to needs review as I found a small edge case after the type hinting changes when the OS mappings are not set.
-
kim.pepper โ
committed e9c0b841 on 3.x authored by
achap โ
Issue #3285438 by achap, varshith, bkildow, kim.pepper, longwave,...
-
kim.pepper โ
committed e9c0b841 on 3.x authored by
achap โ
- Status changed to Fixed
5 months ago 1:42am 17 June 2024 - ๐ฆ๐บAustralia larowlan ๐ฆ๐บ๐.au GMT+10
kim.pepper โ credited larowlan โ .
- ๐ฆ๐บAustralia kim.pepper ๐โโ๏ธ๐ฆ๐บSydney, Australia
Committed to 3.x. Thanks!
-
kim.pepper โ
committed 1e35a267 on 2.x
Issue #3285438 by achap, varshith, bkildow, kim.pepper, longwave,...
-
kim.pepper โ
committed 1e35a267 on 2.x
Automatically closed - issue fixed for 2 weeks with no activity.