-
Notifications
You must be signed in to change notification settings - Fork 3k
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
feat(es-setup): add logic in elasticsearch setup to compare-and-update index if already exists #2312
Conversation
One auxiliary change i have done as part of this PR is to change type of value of attribute, "max_ngram_diff" in settings.json to string. This is how ES internally expects and persists it. |
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 is amazing!! Awesome work!!! Just minor comments below.
Has this script been locally tested as a docker image? Can you share the results?
In the meantime, I will be testing locally as well.
@dexter-mh-lee I have locally tested it as a docker image but unfortunately, i didn't captured screenshots for all the scenarios. I can try to redo those scenarios but i have a better idea to save regression efforts in future also. What are your thoughts on writing BATS test for this script and then integrating it to git ci workflow? |
BATS test would be great. This is the only complicated script we have. I ran dev.sh and it is failing. I think it is failing to run bash scripts. Changing to #!/bin/sh gives me a script syntax error, so the file is definitely there. |
|
Scenario: ES doesn't have any index, running setup for first time
Scenario: ES have indexes, running setup for second time
|
Seems like there's a false positive above for index tagdocument |
Once I ran it after ingesting demo data (no change to mapping/setting),
^ seems like an issue where emails wasn't set correctly in mappings.yml and was dynamically created as the document was indexed.
^ I see that the new indices were created and reindexed correctly, but for some reason, it's failing. Can it be that we are not waiting enough? |
Yes, it was due to empty declaration of tokenizers in tagdocument. I have removed it from settings.yaml. Other 3 such cases are below. As you pointed out, they are coming as data is being dynamically written for fields not declared in mappings.yaml. Do we sync up these also?
and
|
Here, reindexing is not failing. The issue was that brace expansion is not much sh-compatible. So, i have changed it with seq command. |
This is very tricky to detect. Ideally, all documents should be declared in mappings.yaml. I think it's okay for now. |
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.
LGTM. Thanks for making the changes!
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.
LGTM!
Thanks @shakti-garg and @dexter-mh-lee
Checklist
resolves #2310