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

Replace more places accessing Elasticsearch client directly with proxied calls #2016

Merged

Conversation

artem-shelkovnikov
Copy link
Member

@artem-shelkovnikov artem-shelkovnikov commented Dec 22, 2023

Part of https://github.com/elastic/enterprise-search-team/issues/6412

This PR changes more places to use rich Elasticsearch clients instead of direct calls to python Elasticsearch client.

Example methods that were introduced or changed with this PR:

  • ESManagementClient.ensure_exists method got "expand_wildcards" argument that's default to open - some places use "all", but most of the places use "open". "open" is also more conservative, thus the choice
  • ESManagementClient.create_content_index - new method to create content index. Normally kibana does it, but in some cases (mostly CI or CLI) we wanna create the index ourselves. This method is just extracted from the original place that was calling non-rich Elasticsearch client with no changes
  • ESManagementClient.ensure_content_index_mappings - new method to ensure that content index has our search mappings. This method is just extracted from the original place that was calling non-rich Elasticsearch client with no changes
  • ESManagementClient.ensure_ingest_pipeline_exists - new method to ensure that ingest pipeline with provided id exists, if not it's created with provided arguments. Used by kibana.py. This method is just extracted from the original place that was calling non-rich Elasticsearch client with no changes
  • ESManagementClient.delete_indices - new method to delete indices. Used by kibana.py. This method is just extracted from the original place that was calling non-rich Elasticsearch client with no changes
  • ESManagementClient.upsert - new method to upsert a record into target index. Used by kibana.py. This method is just extracted from the original place that was calling non-rich Elasticsearch client with no changes
  • ESManagementClient.yield_existing_documents_metadata - formely Extractor._get_existing_ids

There is still one change I haven't moved - it's call to bulk, I will address it in later PR(s).

All other changes faciliate the changes outlined above.

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)

@artem-shelkovnikov artem-shelkovnikov marked this pull request as ready for review December 22, 2023 14:32
@artem-shelkovnikov artem-shelkovnikov requested a review from a team December 22, 2023 14:32
@@ -30,7 +30,8 @@ async def list_connectors(self):
# TODO move this on top
try:
await self.es_management_client.ensure_exists(
indices=[CONCRETE_CONNECTORS_INDEX, CONCRETE_JOBS_INDEX]
indices=[CONCRETE_CONNECTORS_INDEX, CONCRETE_JOBS_INDEX],
expand_wildcards="all",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think don't need expand_wildcards anymore because connectors work only with one index.

I'm getting rid of the wildcards in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

But this one is creating jobs and connectors indices that are hidden, I believe?

Copy link
Contributor

@vidok vidok Jan 3, 2024

Choose a reason for hiding this comment

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

I think expand_wildcards is needed only when you want to update multiple indices that match the pattern.

Here, we operate only with concrete indices.

https://elasticsearch-py.readthedocs.io/en/v8.11.1/api/indices.html#elasticsearch.client.IndicesClient.exists


async def __create_connector(
self, index_name, service_type, configuration, is_native, language, from_index
):
try:
await self.es_management_client.ensure_exists(
indices=[CONCRETE_CONNECTORS_INDEX, CONCRETE_JOBS_INDEX]
indices=[CONCRETE_CONNECTORS_INDEX, CONCRETE_JOBS_INDEX],
expand_wildcards="all",
Copy link
Contributor

Choose a reason for hiding this comment

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

The same. I don't think we need this.

@vidok
Copy link
Contributor

vidok commented Dec 22, 2023

Great work! I left a couple of comments.

Also, I don't know if it's a Pythonic way, but does it make sense to move ESManagementClient to a new file?

@artem-shelkovnikov artem-shelkovnikov requested a review from a team January 2, 2024 11:25
Copy link
Contributor

@vidok vidok left a comment

Choose a reason for hiding this comment

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

Great work!

@artem-shelkovnikov artem-shelkovnikov enabled auto-merge (squash) January 3, 2024 16:18
@artem-shelkovnikov artem-shelkovnikov merged commit cf5f20c into main Jan 3, 2024
@artem-shelkovnikov artem-shelkovnikov deleted the artem/refactor-more-places-using-client-directly branch January 3, 2024 16:42
Copy link

github-actions bot commented Jan 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 2016 --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.

2 participants