-
-
Notifications
You must be signed in to change notification settings - Fork 214
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
Use ES aliases instead of index names in queries #26733
Conversation
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.
Looks good @sravfeyn
"sms": SMS_INDEX_INFO, | ||
"report_cases": REPORT_CASE_INDEX, | ||
"report_xforms": REPORT_XFORM_INDEX, | ||
"case_search": CASE_SEARCH_INDEX_INFO, |
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.
❤️
Bumping for review on recently added commits. |
@snopoke @dannyroberts Bumping again for review on last 3 commits |
@dannyroberts @snopoke Is this good to go? |
@sravfeyn still some outstanding comments |
@snopoke Sorry, which ones Simon? |
@sravfeyn thanks for the responses, your comments are still pending, you need to click 'submit review' (I've been bitten by that many times). |
|
@snopoke I have responded to the comments (hope you are able to see them) and update the docs per your comment. |
@sravfeyn have these changes been tested on staging at all? |
Yes, they have been live on staging for more than few weeks now. I have tested basic things and made sure requests are coming to aliases and not indices. Also submitted a QA ticket to go along with #28032 |
QA is passed on this https://dimagi-dev.atlassian.net/browse/QA-1532 @snopoke think we can merge this now? |
Hello @dimagi/team-commcare-hq This is a pretty big change in Elasticsearch, so wanted to make an announcement. From this PR on, HQ code will use Elasticsearch aliases to interact with Elasticsearch instead of index names as it previously did. There is also a change in the workflow of changing ES index mappings, please read it here https://github.com/dimagi/commcare-hq/pull/26733/files#diff-55ae021bb3841b645117710c304ad227 This is QA and review pass. I am going to merge it tomorrow (to give some time for people to be aware and raise any concerns if any) |
This is well tested but just to be safe, if any issues with pillows do come up form this PR, this can be reverted. We just need to rewind pillows https://github.com/dimagi/commcare-hq/blob/master/corehq/apps/es/management/commands/restore_es_snapshot.py#L91 |
This is only open to get any feedback and not ready for merge.
We are currently querying ES via index names. Querying via alias is supported and will be needed for ICDS where we are thinking to have multiple indices under one alias. This PR doesn't implement any multiple index support, but as a starting point simply queries ES using alias instead of index in all places that I could find. I have added a logger to find any places that I missed where an index is directly queried instead of an alias.
My plan for this PR is
ENABLE_ES_INTERFACE_LOGGING
After this my plan to actually add multiple index support for single alias. My tentative plan is
write_alias
attribute toElasticsearchIndexInfo
and use that for all write operations.write_alias
points to only single indexalias
attribute ofElasticsearchIndexInfo
to read_alias and use that for allread
operations.This will enable individual envs to manage their ES indices outside of HQ with custom settings, rolling indices etc