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

fix: unblock event loop to allow health service #5433

Merged
merged 6 commits into from Nov 24, 2022
Merged

Conversation

JoanFM
Copy link
Member

@JoanFM JoanFM commented Nov 23, 2022

Goals:
Runs the Executor methods in a protected thread so that it is async and unlocks the event lock so that health service checks can pass

@github-actions github-actions bot added size/M area/cicd This issue/PR affects the cicd pipeline area/core This issue/PR affects the core codebase area/housekeeping This issue/PR is housekeeping area/testing This issue/PR affects testing labels Nov 23, 2022
@codecov
Copy link

codecov bot commented Nov 23, 2022

Codecov Report

Merging #5433 (22d04e3) into master (beecc78) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5433   +/-   ##
=======================================
  Coverage   87.26%   87.26%           
=======================================
  Files          99       99           
  Lines        6566     6566           
=======================================
  Hits         5730     5730           
  Misses        836      836           
Flag Coverage Δ
jina 87.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@github-actions github-actions bot added the area/setup This issue/PR affects setting up Jina label Nov 23, 2022
@JoanFM JoanFM closed this Nov 23, 2022
@JoanFM JoanFM reopened this Nov 23, 2022
@JoanFM JoanFM changed the title test: try keeping only executor in thread changes fix: unblock event loop to allow health service Nov 24, 2022
@JoanFM JoanFM marked this pull request as ready for review November 24, 2022 08:26
@pytest.mark.parametrize(
'docker_images', [['slow-process-executor', 'jinaai/jina']], indirect=True
)
async def test_slow_executor_readiness_probe_works(docker_images, tmpdir, logger):
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand this specific test
why don't we just send requests in a separate process, do port forwarding and execute a manual health check (WorkerRuntime.is_ready).
In another PR that adds liveness probe we can modify this test

Copy link
Member Author

Choose a reason for hiding this comment

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

what u say is done in the test_runtimes. This test adds little value for now. It proves that really slow works. We can remove later, or it will be more important when livenessProbe is there.

Let's keep for now

@JoanFM JoanFM merged commit 8c110d6 into master Nov 24, 2022
@JoanFM JoanFM deleted the executor-thread branch November 24, 2022 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cicd This issue/PR affects the cicd pipeline area/core This issue/PR affects the core codebase area/housekeeping This issue/PR is housekeeping area/setup This issue/PR affects setting up Jina area/testing This issue/PR affects testing component/resource size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants