Skip to content
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

disable _all field for sms index #26515

Merged
merged 6 commits into from
Feb 3, 2020
Merged

Conversation

dannyroberts
Copy link
Member

SUMMARY

Amended @snopoke's commit from #25666 to include only SMS index and to update the index name to trigger a reindex.

On production I think the SMS index is small enough that it reindexing isn't a huge burden. Is that true on ICDS as well?

@dannyroberts dannyroberts added the reindex/migration Reindex or migration will be required during or before deploy label Jan 28, 2020
@dannyroberts dannyroberts added the product/invisible Change has no end-user visible impact label Jan 28, 2020
Copy link
Contributor

@snopoke snopoke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably OK for ICDS (we have 20M docs). One concern is that we haven't done an ES reindex in a long time so may need to tweak it. We can try kick off the reindex in a limited release and see how it goes.

For future reindexes it would be great to figure out the process for using the ES reindex feature: https://www.elastic.co/guide/en/elasticsearch/reference/2.4/docs-reindex.html

@dannyroberts
Copy link
Member Author

@snopoke Cool, yeah I agree that'd be nice. I took a stab at a CEP for this #26516, feel free to update the description directly if you have more fleshed out ideas and/or to comment.

Copy link
Member Author

@dannyroberts dannyroberts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't approve my own PR, but @snopoke I approve your changes. Let me know if you want me to delay, otherwise I'll probably merge this tomorrow (Jan 30)

@snopoke
Copy link
Contributor

snopoke commented Jan 30, 2020

@dannyroberts please can we wait to merge this until the ICDS reindex is complete otherwise it will block deploy. I'm going to continue monitoring it today and hopefully it will be done by EOD.

@snopoke
Copy link
Contributor

snopoke commented Feb 1, 2020

@dannyroberts we can go ahead and merge this now

@dannyroberts
Copy link
Member Author

Awesome, thanks!

@dannyroberts dannyroberts merged commit 0106085 into master Feb 3, 2020
@dannyroberts dannyroberts deleted the dmr/es-sms-disable-all branch February 3, 2020 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants