8000 fix(core): dont respond to cancel and idle by jacobowitz · Pull Request #2940 · jina-ai/serve · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

fix(core): dont respond to cancel and idle #2940

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Jul 13, 2021
Merged

fix(core): dont respond to cancel and idle #2940

merged 5 commits into from
Jul 13, 2021

Conversation

jacobowitz
Copy link
Contributor
@jacobowitz jacobowitz commented Jul 13, 2021 •
edited
Loading

Skip sending responses to IDLE and CANCEL requests.

The test only tests IDLE unfortunately, but CANCEL actually impacts the tested pod so I could not use that one.

Fixes #2930

@jacobowitz jacobowitz requested a review from JoanFM July 13, 2021 12:35
@jacobowitz jacobowitz requested a review from a team as a code owner July 13, 2021 12:35
@jina-bot jina-bot added size/S area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/peapod labels Jul 13, 2021
JoanFM
JoanFM suggested changes Jul 13, 2021
View reviewed changes
@jacobowitz jacobowitz force-pushed the fix-ctrl-response-sending branch from fa2c7e1 to 62bae29 Compare July 13, 2021 12:37
JoanFM
JoanFM suggested changes Jul 13, 2021
View reviewed changes
@jacobowitz jacobowitz changed the title fix(core): dont respond to close and idle fix(core): dont respond to cancel and idle Jul 13, 2021
@codecov
Copy link
codecov bot commented Jul 13, 2021 •
edited
Loading

Codecov Report

Merging #2940 (3c90e2b) into master (d81cd36) will increase coverage by 1.85%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2940      +/-   ##
==========================================
+ Coverage   87.07%   88.92%   +1.85%     
==========================================
  Files         138      138              
  Lines        9567     9569       +2     
==========================================
+ Hits         8330     8509     +179     
+ Misses       1237     1060     -177
Flag Coverage Δ
daemon 43.26% <100.00%> (+0.01%) ⬆️
jina 88.91% <100.00%> (+1.85%) ⬆️

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

Impacted Files Coverage Δ
jina/peapods/runtimes/zmq/zed.py 92.73% <100.00%> (+0.92%) ⬆️
jina/peapods/peas/__init__.py 95.88% <0.00%> (+1.76%) ⬆️
jina/jaml/helper.py 85.00% <0.00%> (+2.50%) ⬆️
jina/jaml/__init__.py 95.09% <0.00%> (+3.92%) ⬆️
jina/enums.py 95.42% <0.00%> (+4.57%) ⬆️
jina/checker.py 96.55% <0.00%> (+10.34%) ⬆️
jina/types/request/__init__.py 99.13% <0.00%> (+13.79%) ⬆️
jina/types/ndarray/dense/numpy.py 100.00% <0.00%> (+15.38%) ⬆️
jina/helper.py 80.41% <0.00%> (+15.81%) ⬆️
... and 2 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 d81cd36...3c90e2b. Read the comment docs.

JoanFM
JoanFM suggested changes Jul 13, 2021
View reviewed changes
@jacobowitz jacobowitz merged commit d28b279 into master Jul 13, 2021
@jacobowitz jacobowitz deleted the fix-ctrl-response-sending branch July 13, 2021 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
69D2

@JoanFM JoanFM JoanFM approved these changes

@maximilianwerk maximilianwerk Awaiting requested review from maximilianwerk

Assignees
No one assigned
Labels
area/core This issue/PR affects the core codebase area/network This issue/PR affects network functionality area/testing This issue/PR affects testing component/peapod size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dont send responses for some control messages
3 participants
@jacobowitz @JoanFM @jina-bot
0