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

feat: use single gateway streamer for CompositeGateway #5598

Merged
merged 9 commits into from
Jan 16, 2023

Conversation

girishc13
Copy link
Contributor

@girishc13 girishc13 self-assigned this Jan 13, 2023
@github-actions github-actions bot added size/XS area/core This issue/PR affects the core codebase labels Jan 13, 2023
@codecov
Copy link

codecov bot commented Jan 13, 2023

Codecov Report

Merging #5598 (ee87a8f) into master (e5b203e) will increase coverage by 1.47%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #5598      +/-   ##
==========================================
+ Coverage   86.52%   87.99%   +1.47%     
==========================================
  Files         124      124              
  Lines       10069    10070       +1     
==========================================
+ Hits         8712     8861     +149     
+ Misses       1357     1209     -148     
Flag Coverage Δ
jina 87.99% <100.00%> (+1.47%) ⬆️

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

Impacted Files Coverage Δ
jina/serve/runtimes/gateway/composite/gateway.py 100.00% <100.00%> (ø)
jina/orchestrate/deployments/__init__.py 89.77% <0.00%> (-0.32%) ⬇️
jina/orchestrate/flow/base.py 90.08% <0.00%> (+0.36%) ⬆️
jina/serve/networking.py 90.45% <0.00%> (+1.23%) ⬆️
jina/helper.py 80.93% <0.00%> (+1.28%) ⬆️
...ina/serve/runtimes/gateway/graph/topology_graph.py 99.48% <0.00%> (+1.54%) ⬆️
jina/serve/streamer.py 94.68% <0.00%> (+2.12%) ⬆️
jina/serve/runtimes/worker/__init__.py 95.74% <0.00%> (+2.12%) ⬆️
jina/clients/base/http.py 95.83% <0.00%> (+2.77%) ⬆️
jina/serve/runtimes/gateway/http/app.py 97.81% <0.00%> (+2.91%) ⬆️
... and 6 more

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

@girishc13 girishc13 marked this pull request as ready for review January 13, 2023 15:35
@girishc13 girishc13 marked this pull request as draft January 13, 2023 15:47
Copy link
Member

@JoanFM JoanFM left a comment

Choose a reason for hiding this comment

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

make sure the close method is safe

@girishc13 girishc13 marked this pull request as ready for review January 16, 2023 11:58
@alaeddine-13
Copy link
Contributor

make sure the close method is safe

I believe it's fine without a lock. The streamer.close is already called once, when the runtime is shutdown

@girishc13
Copy link
Contributor Author

The lock is not necessary since the GatewayRuntime handles the gateway.streamer.close() method so there is no need to synchronize between multiple Gateway instances.

@JoanFM JoanFM merged commit 72f2eed into master Jan 16, 2023
@JoanFM JoanFM deleted the feat-runtimes-5592 branch January 16, 2023 15:02
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 size/S size/XS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: use single GatewayStreamer object for all gateways in the CompositeGateway
3 participants