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

Revert email analyzer #32

Merged
merged 2 commits into from Feb 4, 2020
Merged

Revert email analyzer #32

merged 2 commits into from Feb 4, 2020

Conversation

izakp
Copy link
Contributor

@izakp izakp commented Feb 3, 2020

@anandaroop pointed out recently that our Elasticsearch index settings differ on staging and production and the email analyzer was never applied to production indices.

I believe that this change was introduced in error and has no effect due to its low position (after ngram) in the analysis pipeline.

cc @cavvia

@izakp izakp changed the title Revert email analyzier Revert email analyzer Feb 3, 2020
@ArtsyOpenSource
Copy link

1 Warning
⚠️ Unless you’re refactoring existing code, please update CHANGELOG.md.

Here's an example of a CHANGELOG.md entry:

* [#32](https://github.com/artsy/estella/pull/32): Revert email analyzer - [@izakp](https://github.com/izakp).

Generated by 🚫 Danger

Copy link
Contributor

@joeyAghion joeyAghion left a comment

Choose a reason for hiding this comment

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

I don't have much context for why the email analyzer was added in the first place (back during the v5 upgrade in #29). Is it possible some indices do use it, despite the higher-priority ngram analyzer?

@mzikherman
Copy link
Contributor

Well, if it's not actually enabled or used in production, might as well remove it here!

Copy link
Member

@anandaroop anandaroop left a comment

Choose a reason for hiding this comment

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

I was wondering if might have been intended to support look up Users by email or something. But it sounds like it hasn't been used or missed, so I'm down to merge this.

@izakp
Copy link
Contributor Author

izakp commented Feb 4, 2020

@joeyAghion I think it was my mistake to add this. Most of our email fields are not analyzed.

@joeyAghion
Copy link
Contributor

Seems like everyone's in agreement, so I'll go ahead and merge. The staging index might require a manual migration, if I understand correctly, right?

@joeyAghion joeyAghion merged commit c21c0de into artsy:master Feb 4, 2020
@izakp
Copy link
Contributor Author

izakp commented Feb 4, 2020

@joeyAghion this will take an update of Estella and a version bump in Gravity. I think we should let the weekly reindex take care of this as indices are recreated

@cavvia
Copy link
Contributor

cavvia commented Feb 25, 2020

Admin does still allow for search by email but it uses default and ngram analyzer and works fine I think, so there's no need for this analyzer currently.

@izakp
Copy link
Contributor Author

izakp commented Mar 2, 2020

Admin does still allow for search by email but it uses default and ngram analyzer and works fine I think, so there's no need for this analyzer currently.

@cavvia right - this was an overoptimization on my part so let's release Estella and upgrade Gravity - the next staging reindex should clean up the index settings

@izakp izakp mentioned this pull request Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants