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

refactor: refactor is ready #2833

Merged
merged 2 commits into from
Jul 2, 2021
Merged

refactor: refactor is ready #2833

merged 2 commits into from
Jul 2, 2021

Conversation

JoanFM
Copy link
Member

@JoanFM JoanFM commented Jul 2, 2021

Changes introduced
We are currently setting is_ready before the runtime is actually really ready to handle traffic. This PR gives the responsability to set is_ready flag to the runtime and tries to make sure that is_ready is more meaningful.

It also allows us to avoid the need to call terminate and have a potentially more grateful shutdown

@JoanFM JoanFM requested a review from a team as a code owner July 2, 2021 06:20
@jina-bot jina-bot added size/M area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality component/peapod labels Jul 2, 2021
@codecov
Copy link

codecov bot commented Jul 2, 2021

Codecov Report

Merging #2833 (ca8d039) into master (0094bbf) will increase coverage by 6.88%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2833      +/-   ##
==========================================
+ Coverage   82.21%   89.10%   +6.88%     
==========================================
  Files         106      138      +32     
  Lines        7064     9371    +2307     
==========================================
+ Hits         5808     8350    +2542     
+ Misses       1256     1021     -235     
Flag Coverage Δ
daemon 47.15% <ø> (?)
jina 88.50% <ø> (?)

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

Impacted Files Coverage Δ
jina/__init__.py 71.64% <ø> (ø)
jina/checker.py 96.55% <ø> (ø)
jina/clients/__init__.py 100.00% <ø> (ø)
jina/clients/base/__init__.py 90.90% <ø> (ø)
jina/clients/base/grpc.py 63.82% <ø> (ø)
jina/clients/base/http.py 93.02% <ø> (ø)
jina/clients/base/websocket.py 86.36% <ø> (ø)
jina/clients/grpc.py 100.00% <ø> (ø)
jina/clients/helper.py 100.00% <ø> (ø)
jina/clients/http.py 100.00% <ø> (ø)
... and 215 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bca2c1...ca8d039. Read the comment docs.

jacobowitz
jacobowitz previously approved these changes Jul 2, 2021
Copy link
Contributor

@jacobowitz jacobowitz left a comment

Choose a reason for hiding this comment

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

lgtm, we may add some wrapper later in the base runtime to hide the event implementation from the concrete runtimes, but that does not change too much.

Copy link
Contributor

@jacobowitz jacobowitz left a comment

Choose a reason for hiding this comment

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

Actually there are some tests failing which look legit: https://github.com/jina-ai/jina/pull/2833/checks?check_run_id=2969742801

@jina-bot jina-bot added the area/testing This issue/PR affects testing label Jul 2, 2021
@JoanFM JoanFM requested a review from jacobowitz July 2, 2021 07:55
@jacobowitz jacobowitz merged commit dc5ab2b into master Jul 2, 2021
@jacobowitz jacobowitz deleted the refactor-is-ready branch July 2, 2021 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core This issue/PR affects the core codebase area/helper This issue/PR affects the helper functionality area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/peapod size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants