fix: loadbalance stream based on response #6122

NarekA · 2023-12-04T20:01:19Z

The current load balancer assumes all GET requests are streaming and all POST requests are not. This may not be true for user-added fast-api endpoints and in the past we have talked about using POST for streaming. (One of the benefits of this is the Swagger UI better documents the payload for POST endpoints).

It makes more sense to use the response content-type to determine when to stream.

Goals:

Allow Post streaming POST endpoints and non-streaming GET endpoints
check and update documentation. See guide and ask the team.

codecov · 2023-12-04T20:07:24Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c2a7c2) 73.86% compared to head (bc91179) 75.18%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6122      +/-   ##
==========================================
+ Coverage   73.86%   75.18%   +1.32%     
==========================================
  Files         152      152              
  Lines       14069    14062       -7     
==========================================
+ Hits        10392    10573     +181     
+ Misses       3677     3489     -188

Flag	Coverage Δ
jina	`75.18% <100.00%> (+1.32%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JoanFM · 2023-12-04T20:51:04Z

Hey @NarekA,

Thanks for the contribution, can you check the failed tests and add a test showing what u are intending to fix?

NarekA · 2023-12-04T21:00:59Z

@JoanFM I think the issue came about because I was using request.content_type instead of response.content_type. I think the latest commit should fix it.

cenhao · 2023-12-05T01:07:06Z

jina/serve/runtimes/gateway/request_handling.py

+
+                async with _RequestContextManager(
+                    session._request(request.method, target_url, **request_kwargs)
+                ) as response:


@NarekA why not directly use the request method from the session?, it's already wrapping the context manager for you.

def request( self, method: str, url: StrOrURL, **kwargs: Any ) -> "_RequestContextManager": """Perform HTTP request.""" return _RequestContextManager(self._request(method, url, **kwargs)) ```

Somehow I missed this, will fix.

cenhao · 2023-12-05T01:15:31Z

jina/serve/runtimes/gateway/request_handling.py

+                request_kwargs = {
+                    'headers': request.headers,
+                    'params': request.query,
+                }


The original code did not forward the headers & query, which makes me wonder if it's intentional?

it was not intentional

cenhao · 2023-12-05T01:16:54Z

jina/serve/runtimes/gateway/request_handling.py

+                    return web.Response(
+                        body=content,
+                        status=response.status,
+                        content_type=response.content_type,


should the header be added here as well?

Also by looking at the original GET implementation, I wonder if we just need to use StreamResponse all the way. Basically the load balancer just stream whatever it receives out.

yes this would be a good idea

JoanFM · 2023-12-05T08:52:29Z

Most of the errors on tests will be fixed by #6124

cenhao · 2023-12-05T22:59:20Z

jina/serve/runtimes/gateway/request_handling.py

+                    return web.Response(
+                        body=content,
+                        status=response.status,
+                        content_type=response.content_type,


Also by looking at the original GET implementation, I wonder if we just need to use StreamResponse all the way. Basically the load balancer just stream whatever it receives out.

cenhao · 2023-12-05T23:03:41Z

jina/serve/runtimes/gateway/request_handling.py

-                        if payload:
-                            request_kwargs['json'] = payload
-                    except Exception:
-                        self.logger.debug('No JSON payload found in request')


Looking at the original implementation, i have this idea:
it looks like the original logic was only to write a debug log which is not useful at all for production application. Can we just act as a pure proxy here for performance consideration? Something like:

async with session.request(request.method, data=request.iter_any(), **request_kwargs) as response: ....

@NarekA @JoanFM

For some reason, if I try to pass the content in any other way besides the json field, I get an error here. I've tried everything at this point, if you can get this to work, I am interested.

what error do you see?

NarekA · 2023-12-06T16:09:58Z

@JoanFM I'm finding these tests to be prohibitively difficult to debug. Any idea what is going wrong? I need a better way of replicating them locally.

cenhao · 2023-12-06T18:00:16Z

jina/serve/runtimes/gateway/request_handling.py

+                    self.logger.debug('No JSON payload found in request')
+
+                async with session.request(
+                    request.method, url=target_url, **request_kwargs


You might consider adding auto_decompress=False here. I've seen issue when upstream compresses the response, without this flag, together with the upstream header, there will be an error.

Thanks, just fixed this.

Looks like this fixed everything!

JoanFM · 2023-12-06T19:03:40Z

Thanks for this great contribution!

NarekA and others added 2 commits December 4, 2023 11:43

fix: loadbalance right method

af64005

Merge branch 'master' into fix-loadbalancer-forwarding-method

7932b0d

NarekA force-pushed the fix-loadbalancer-forwarding-method branch from e0a8118 to d287848 Compare December 4, 2023 21:11

cenhao reviewed Dec 5, 2023

View reviewed changes

NarekA and others added 4 commits December 5, 2023 13:16

docs: add comment for content type

4b85741

fix: use response content

8292fcf

Merge branch 'master' into fix-loadbalancer-forwarding-method

f170efb

fix: use correct protocol

42f444a

NarekA force-pushed the fix-loadbalancer-forwarding-method branch from 02d3979 to 42f444a Compare December 5, 2023 21:16

NarekA added 2 commits December 5, 2023 13:24

fix: user session.request

7fbe42f

fix: passing json

676ca4c

cenhao reviewed Dec 5, 2023

View reviewed changes

NarekA added 2 commits December 6, 2023 05:10

fix: always stream response

ac1febe

fix: don't pass request headers

c94ea5f

cenhao reviewed Dec 6, 2023

View reviewed changes

fix: disable autodecompress

bc91179

JoanFM approved these changes Dec 6, 2023

View reviewed changes

JoanFM merged commit 129b7b3 into jina-ai:master Dec 6, 2023
131 checks passed

NarekA deleted the fix-loadbalancer-forwarding-method branch December 6, 2023 19:03

JoanFM mentioned this pull request Dec 14, 2023

Release Notes (3.23.2) #6129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: loadbalance stream based on response #6122

fix: loadbalance stream based on response #6122

NarekA commented Dec 4, 2023 •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading

JoanFM commented Dec 4, 2023

NarekA commented Dec 4, 2023

cenhao Dec 5, 2023

NarekA Dec 5, 2023

cenhao Dec 5, 2023

JoanFM Dec 5, 2023

cenhao Dec 5, 2023

cenhao Dec 5, 2023

JoanFM Dec 5, 2023

JoanFM commented Dec 5, 2023

cenhao Dec 5, 2023

cenhao Dec 5, 2023

NarekA Dec 6, 2023

cenhao Dec 6, 2023

NarekA commented Dec 6, 2023

cenhao Dec 6, 2023

NarekA Dec 6, 2023

NarekA Dec 6, 2023

JoanFM commented Dec 6, 2023

fix: loadbalance stream based on response #6122

fix: loadbalance stream based on response #6122

Conversation

NarekA commented Dec 4, 2023 • edited Loading

codecov bot commented Dec 4, 2023 • edited Loading

Codecov Report

JoanFM commented Dec 4, 2023

NarekA commented Dec 4, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanFM commented Dec 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NarekA commented Dec 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoanFM commented Dec 6, 2023

NarekA commented Dec 4, 2023 •

edited

Loading

codecov bot commented Dec 4, 2023 •

edited

Loading