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

Stop providing mappings/settings for data indices #3013

Merged
merged 1 commit into from
Dec 3, 2024

Conversation

seanstory
Copy link
Member

Closes #2962

Since their inception, Connectors have ensured that the data indices they write to use specific mappings/settings that were optimized for Elastic App Search. On their own (without App Search) these mappings/settings are likely to be pretty inefficient in terms of time and space, and don't follow Elastic conventions for default mappings/settings/subfield names.

The 9.0 major version bump is the right time to remove this opinionated approach to our output indices. Going forward, connectors should strive to be more agnostic. They will output JSON data. When customers want to customize their mappings, they will be able to do so from Elastic defaults, rather than the more arcane legacy App Search defaults.

This is a breaking change for anyone who expects an index created by connectors in 9.x to behave the same as one created in 8.x.

Checklists

Pre-Review Checklist

  • this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • this PR has a thorough description
  • Covered the changes with automated tests
  • Tested the changes locally
  • Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)

Release Note

Connectors will no longer be opinionated about output index mappings/settings. The Elastic default mappings/settings will be used after this change, rather than Connectors attempting to set their own convention of mappings/settings that were optimized for the legacy App Search product. If the old mappings/settings were desirable, they can still be set in advance by creating the index first, before pointing a connector to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is the key piece of this PR. Removing this file in its entirety. All the rest of the diff is just adjusting to removing this code.

Comment on lines +10 to +11
TIMESTAMP_FIELD = "_timestamp"
DEFAULT_LANGUAGE = "en"
Copy link
Member Author

Choose a reason for hiding this comment

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

These constants were the only pieces from settings.py that seemed worth keeping.

We could go farther and remove language as a concept from connectors as well. However, this would require API changes and a change to the Protocol ("language" field in the connector record), as well as a migration to remove it, so I'm opting to keep that vestigial for now.

Copy link
Member

@artem-shelkovnikov artem-shelkovnikov left a comment

Choose a reason for hiding this comment

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

🚀 🌒

Copy link
Member

@jedrazb jedrazb left a comment

Choose a reason for hiding this comment

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

@seanstory Sanity check - without default index mappings and settings, should we expect the framework to still work? If we create the connector via API, I’m pretty sure that index created by framework, with no "default" mappings will cause a failure.

At least that ^ was the case a while ago when I tried to attach a plain my-index via API and that was manually created with PUT my-index

Not in scope: When creating the content index in Kibana, we also apply default mappings/settings—should we clean this up as well?

@jedrazb
Copy link
Member

jedrazb commented Dec 3, 2024

This is a breaking change for anyone who expects an index created by connectors in 9.x to behave the same as one created in 8.x.

Should this be presented to breakign changes comittee?

@seanstory
Copy link
Member Author

seanstory commented Dec 3, 2024

Should this be presented to breaking changes committee?

@joemcelroy is covering that as part of https://github.com/elastic/dev/issues/2880. This PR is just one of several that will go in to remove our usage of these mappings across the Search products.

Not in scope: When creating the content index in Kibana, we also apply default mappings/settings—should we clean this up as well?

I believe @joemcelroy is also on that. :) It's wider than just connector indices in Kibana, but all indices we create through that page, and he'll cover all use cases in one pass.

without default index mappings and settings, should we expect the framework to still work? If we create the connector via API, I’m pretty sure that index created by framework, with no "default" mappings will cause a failure.

I've just double-checked and had no issue

  1. created connector with:
PUT _connector/my-connector
{
  "index_name": "search-google-drive",
  "name": "My Connector",
  "service_type": "google_drive"
}
  1. did nothing to create search-google-drive
  2. configured the connector, ran a sync
Screenshot 2024-12-03 at 8 31 42 AM

As expected, the mappings are now Elastic defaults. Sync was successful.

@seanstory seanstory merged commit c1773fe into main Dec 3, 2024
5 checks passed
@seanstory seanstory deleted the seanstory/2962-remove-mappings branch December 3, 2024 14:35
Copy link

github-actions bot commented Dec 3, 2024

💔 Failed to create backport PR(s)

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 3013 --autoMerge --autoMergeMethod squash

@jedrazb
Copy link
Member

jedrazb commented Dec 3, 2024

Yup @seanstory, I recreated this and it worked! Nice 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove App Search settings/mappings
3 participants