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: add grpc reflection support #4585

Merged
merged 11 commits into from
Apr 11, 2022
Merged

feat: add grpc reflection support #4585

merged 11 commits into from
Apr 11, 2022

Conversation

jacobowitz
Copy link
Contributor

@jacobowitz jacobowitz commented Apr 5, 2022

This PR allows external Executor to be fronted by our gRPC Gateway. This enables us to deploy external Executors together with a Gateway (as Executor.serve() is doing.). In theory this also allows us to integrate complete Flows as external Executors into another Flow. See the tests for some examples.

Implementation details:
I've added support for gRPC reflection with this PR. When we add a new connection, we first check the available gRPC endpoints using reflection. This way we can figure out automatically if the GrpcConnctionPool is used to target a Worker/Head or Gateway. This is necessary because they have different gRPC interfaces (streaming and non streaming). There is no configuration necessary to enable this. The reflection call is done lazily before the first request is send to the target.

  • Support reflection via grpc. Find out if the target is gateway or head/worker. Create the right stubs and hide this in the GrpcConnectionPool
  • Add test case for the g2g scenario
  • Check on parsing that merging is not required at the external Executor
  • Mention merge limitations in docs

Closes #4556

Unblocks: #4502

@github-actions github-actions bot added size/M area/core This issue/PR affects the core codebase area/setup This issue/PR affects setting up Jina area/testing This issue/PR affects testing labels Apr 5, 2022
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

Latency summary

Current PR yields:

  • 🐎🐎🐎🐎 index QPS at 1156, delta to last 2 avg.: +11%
  • 🐎🐎🐎🐎 query QPS at 64, delta to last 2 avg.: +23%
  • 🐢🐢 avg flow time within 1.0674 seconds, delta to last 2 avg.: -33%
  • 😶 import jina within 0.468 seconds, delta to last 2 avg.: -2%

Breakdown

Version Index QPS Query QPS Avg Flow Time (s) Import Time (s)
current 1156 64 1.0674 0.468
3.2.10 996 50 1.6439 0.5042
3.2.9 1078 53 1.5694 0.4579

Backed by latency-tracking. Further commits will update this comment.

@codecov
Copy link

codecov bot commented Apr 7, 2022

Codecov Report

Merging #4585 (09902bf) into master (bdad931) will increase coverage by 0.00%.
The diff coverage is 90.12%.

@@           Coverage Diff           @@
##           master    #4585   +/-   ##
=======================================
  Coverage   87.70%   87.71%           
=======================================
  Files         117      117           
  Lines        8411     8464   +53     
=======================================
+ Hits         7377     7424   +47     
- Misses       1034     1040    +6     
Flag Coverage Δ
jina 87.71% <90.12%> (+0.02%) ⬆️

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

Impacted Files Coverage Δ
jina/serve/networking.py 86.59% <87.69%> (+0.83%) ⬆️
jina/orchestrate/flow/base.py 89.90% <100.00%> (+0.03%) ⬆️
...ina/serve/runtimes/gateway/graph/topology_graph.py 100.00% <100.00%> (ø)
jina/serve/runtimes/gateway/grpc/__init__.py 94.73% <100.00%> (+0.29%) ⬆️
jina/serve/runtimes/head/__init__.py 92.59% <100.00%> (+0.16%) ⬆️
jina/serve/runtimes/worker/__init__.py 94.11% <100.00%> (+0.27%) ⬆️
...a/orchestrate/deployments/config/docker_compose.py 98.97% <0.00%> (-1.03%) ⬇️
.../runtimes/request_handlers/data_request_handler.py 92.10% <0.00%> (-0.88%) ⬇️

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 bdad931...09902bf. Read the comment docs.

@github-actions github-actions bot added the size/L label Apr 7, 2022
@github-actions github-actions bot added the area/docs This issue/PR affects the docs label Apr 8, 2022
@jacobowitz jacobowitz requested a review from JoanFM April 8, 2022 13:19
@jacobowitz jacobowitz marked this pull request as ready for review April 8, 2022 13:19
jina/serve/networking.py Show resolved Hide resolved
jina/serve/networking.py Outdated Show resolved Hide resolved
async def send_requests(
self, requests: List[Request], metadata, compression
) -> Tuple:
if not self._initialized:
Copy link
Member

Choose a reason for hiding this comment

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

can't we avoid this and simply initialize at __init__ time? I prefer a more stateless approach

Copy link
Contributor Author

Choose a reason for hiding this comment

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

unfortunately not. The lazy init here is connecting to the server's grpc reflection endpoint, but the init is called on adding connections. At this time the server might not be ready yet. This is why I am delaying this until sending of the first data request. Then the server should be ready (or the request would fail anyway).

jina/serve/networking.py Show resolved Hide resolved
@github-actions
Copy link

📝 Docs are deployed on https://feat-g2g-external--jina-docs.netlify.app 🎉

@jacobowitz jacobowitz requested a review from JoanFM April 11, 2022 08:46
@JoanFM JoanFM merged commit f45a6e9 into master Apr 11, 2022
@JoanFM JoanFM deleted the feat-g2g-external branch April 11, 2022 13:24
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/docs This issue/PR affects the docs area/setup This issue/PR affects setting up Jina area/testing This issue/PR affects testing component/resource size/L size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Gateway to Gateway communication
3 participants