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

flow_control: Refactor setWatermark #36738

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

tyxia
Copy link
Member

@tyxia tyxia commented Oct 21, 2024

Context
In upcoming flow control part 3 PR where downstream pushes back sidestream, sidestream needs to subscribe to downstream watermark events (via addDownstreamWatermarkCallbacks), so that back pressure can be invoked when when the downstream buffers are overrun

Problem Statement
The subscription(i.e.,addDownstreamWatermarkCallbacks)happens on pool ready which is initiated by client (e.g., ext_proc)'s startStream. However, the setWatermark happens after it (one line below). This leads to problem that addDownstreamWatermarkCallbacks in sidestream failed because watermark is not set

Tracing example of the repro :
image

Solution

Move setWatermark earlier right after stream has been initiated/started successfully . The watermark callbacks are passed by streamOptions

Risk Level: LOW? runtime gard.
Testing: ext_proc + gRPC integration test
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: tyxia <[email protected]>
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #36738 was opened by tyxia.

see: more, trace.

@tyxia tyxia changed the title wip flow_control: Refactor setWatermark Oct 24, 2024
Signed-off-by: tyxia <[email protected]>
@tyxia tyxia marked this pull request as ready for review October 24, 2024 11:58
@tyxia
Copy link
Member Author

tyxia commented Oct 24, 2024

/assign @alyssawilk @yanavlasov

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants