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

Add logic to send a ping to connector's backend every loop #2105

Merged
merged 6 commits into from
Feb 7, 2024

Conversation

artem-shelkovnikov
Copy link
Member

@artem-shelkovnikov artem-shelkovnikov commented Jan 31, 2024

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

Additionally to running RCF validation (see #2093) every time we check connector for whether the job should be scheduled, now we send a ping to the backend of this connector as well to verify, that it's available.

Before this logic would happen only before sync, which is too late.

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)

Related Pull Requests

Release Note

Now configurations checks health of 3rd-party system periodically and reports errors to Kibana. Previously it was only done before the sync is started.

Copy link
Member

@seanstory seanstory left a comment

Choose a reason for hiding this comment

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

two small nits

data_source.validate_config_fields()
await data_source.validate_config()

connector.log_debug("Pinging the backend")
await data_source.ping()
Copy link
Member

Choose a reason for hiding this comment

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

Should we try/catch for a NotImplementedError? And just skip past it if it's not implemented? https://github.com/elastic/connectors/blob/main/connectors/source.py#L557-L562

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually have data sources, which do not implement ping? Otherwise I would either keep it like that or log a clear error that ping is missing, if we expect it from every data source.

Copy link
Member Author

@artem-shelkovnikov artem-shelkovnikov Feb 2, 2024

Choose a reason for hiding this comment

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

We currently have no sources that do not implement ping.

I copied the logic from https://github.com/elastic/connectors/blob/main/connectors/sync_job_runner.py#L129-L134:

            self.sync_job.log_debug("Validating configuration")
            self.data_provider.validate_config_fields()
            await self.data_provider.validate_config()

            self.sync_job.log_debug("Pinging the backend")
            await self.data_provider.ping()

I'd like to keep logic similar between both places, then if needed refactor and introduce NotImplementedError handling.

Co-authored-by: Sean Story <sean.j.story@gmail.com>
Copy link
Contributor

@navarone-feekery navarone-feekery left a comment

Choose a reason for hiding this comment

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

LGTM

@artem-shelkovnikov artem-shelkovnikov merged commit 038e472 into main Feb 7, 2024
2 checks passed
@artem-shelkovnikov artem-shelkovnikov deleted the artem/add-job-scheduling-ping-every-loop branch February 7, 2024 14:51
Copy link

github-actions bot commented Feb 7, 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 2105 --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.

4 participants