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

Create a new content index if it doesn't exist #1668

Merged
merged 10 commits into from
Sep 27, 2023

Conversation

vidok
Copy link
Contributor

@vidok vidok commented Sep 22, 2023

This PR brings back the original functionality that creates content indices.

This handles a use-case when customers use Connectors without Kibana.

Checklists

Pre-Review Checklist

  • this PR has a meaningful title
  • this PR links to all relevant github issues that it fixes or partially addresses
  • if there is no GH issue, please create it. Each PR should have a link to an issue
  • 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)
  • Considered corresponding documentation changes

@vidok vidok added the v8.11.0 label Sep 22, 2023
@vidok vidok requested a review from a team September 22, 2023 13:34
@vidok vidok marked this pull request as ready for review September 22, 2023 13:38
connectors/es/sink.py Outdated Show resolved Hide resolved
connectors/es/sink.py Outdated Show resolved Hide resolved
connectors/es/sink.py Show resolved Hide resolved
else:
raise IndexMissing(f"Index {index} does not exist!")
self._logger.debug("Index %s already has mappings. Skipping...", index)
Copy link
Member

Choose a reason for hiding this comment

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

I see you mentioned
https://github.com/elastic/connectors-python/blob/6b700bcdbc80e0344dc4b809db9491657df0e3fa/connectors/es/sink.py#L670

Do you want to handle mapping changes here? In normal cases, the index should be created by either Kibana or Connector service. If we find a discrepancy, should we 1) create a new index with the correct mapping or 2) raise an error or 3) simply log a warning message and continue?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question:

If the connector can't apply mappings to the index it should fail immediately. This can already happen, see this issue.

I guess it would be nice to handle it and pass a human-readable error. Is this what you also meant?

@vidok vidok requested review from wangch079 and a team September 25, 2023 13:15
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.

Looks good, left a couple suggestions for log lines.

I personally find log lines ending with "..." suitable only for lines that imply some lengthy process after, e.g:

Creating something...
Created!

connectors/es/sink.py Outdated Show resolved Hide resolved
connectors/es/sink.py Outdated Show resolved Hide resolved
connectors/es/sink.py Outdated Show resolved Hide resolved
connectors/es/sink.py Outdated Show resolved Hide resolved
connectors/es/sink.py Outdated Show resolved Hide resolved
@vidok vidok force-pushed the dmitrii/5767-create-search-index branch from c408d85 to d66c589 Compare September 26, 2023 12:52
@vidok vidok requested review from artem-shelkovnikov and a team September 26, 2023 12:53
@vidok vidok merged commit d7f22f5 into main Sep 27, 2023
@vidok vidok deleted the dmitrii/5767-create-search-index branch September 27, 2023 12:57
@github-actions
Copy link

💔 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 1668 --autoMerge --autoMergeMethod squash

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.

3 participants