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: rolling update and hanging fix #2869

Merged
merged 22 commits into from
Jul 8, 2021
Merged

fix: rolling update and hanging fix #2869

merged 22 commits into from
Jul 8, 2021

Conversation

JoanFM
Copy link
Member

@JoanFM JoanFM commented Jul 7, 2021

Changes introduced

  • Change the CompoundPod exit order to ExitStack to avoid sendind CANCEL to a closed HeadPea

  • Avoid sending IDLE while flushing in_socket

  • Retry sending control signal from Pea to runtime

  • Avoid copying Pea to new process by passing needed arguments. I think is dangerous to copy specially Logger with all the handles

@github-actions
Copy link

github-actions bot commented Jul 7, 2021

This PR closes: #2841

@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality component/peapod labels Jul 7, 2021
@codecov
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #2869 (9b30d67) into master (0094bbf) will increase coverage by 5.80%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2869      +/-   ##
==========================================
+ Coverage   82.21%   88.02%   +5.80%     
==========================================
  Files         106      138      +32     
  Lines        7064     9500    +2436     
==========================================
+ Hits         5808     8362    +2554     
+ Misses       1256     1138     -118     
Flag Coverage Δ
daemon 43.42% <ø> (?)
jina 87.96% <ø> (?)

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.18% <ø> (ø)
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 72e362d...9b30d67. Read the comment docs.

@jina-bot jina-bot added size/M area/cicd This issue/PR affects the cicd pipeline area/housekeeping This issue/PR is housekeeping and removed size/S labels Jul 7, 2021
@jina-bot jina-bot added the area/testing This issue/PR affects testing label Jul 7, 2021
@JoanFM JoanFM force-pushed the fix-2841 branch 4 times, most recently from 5e94078 to 6e19483 Compare July 7, 2021 08:49
@JoanFM JoanFM marked this pull request as ready for review July 7, 2021 16:24
@JoanFM JoanFM requested a review from a team as a code owner July 7, 2021 16:24
@JoanFM JoanFM closed this Jul 7, 2021
@JoanFM JoanFM reopened this Jul 7, 2021
@JoanFM JoanFM requested a review from deepankarm July 8, 2021 06:33
Copy link
Member

@maximilianwerk maximilianwerk left a comment

Choose a reason for hiding this comment

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

LGTM. Anyhow, haven't gone into great detail understanding all consequences.

# if Address already in use `self.in_sock_type` is not set
if (
not self.is_closed
and hasattr(self, 'in_sock_type')
Copy link
Member

Choose a reason for hiding this comment

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

This can be removed again. I fixed the resulting error in #2853 via making sure, this is always set to None in the first place.

Copy link
Member Author

Choose a reason for hiding this comment

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

will remove in follow-up PR if that's okey

@JoanFM JoanFM merged commit 515c0b2 into master Jul 8, 2021
@JoanFM JoanFM deleted the fix-2841 branch July 8, 2021 06:45
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/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

4 participants