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.

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

fix(firehose): Set connection window size to the maximum #3818

Merged
merged 2 commits into from
Aug 8, 2022
Merged

fix(firehose): Set connection window size to the maximum #3818

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
381781c
fix(firehose): http2_adaptive_window
leoyvens Aug 5, 2022
daa4222
firehose: Set connection window size to the maximum
leoyvens Aug 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion graph/src/firehose/endpoints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,9 @@ impl FirehoseEndpoint {
.expect("TLS config on this host is invalid"),
_ => panic!("invalid uri scheme for firehose endpoint"),
}
.connect_timeout(Duration::from_secs(10));
.connect_timeout(Duration::from_secs(10))
.http2_keep_alive_interval(Duration::from_secs(30))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is too low IMHO. While I didn't experience disconnections with this setting, some backends are very restrictive on this (at 10secs, it DOES generate similar disconnections as with http2_adaptive_window)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed the keep alive for now, to focus this PR on the stalling bug.

.http2_adaptive_window(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes disconnections when connecting to firehose through a GCP Load Balancer. (error reading a body from connection: unexpected end of file)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for testing that! I've removed this, and took a different approach of setting a very large window size.


let uri = endpoint.uri().to_string();
//connect_lazy() used to return Result, but not anymore, that makes sence since Channel is not used immediatelly
Expand Down