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

Preserve default Http2Settings when Http2SettingsBuilder is used #3092

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

idelpivnitskiy
Copy link
Member

@idelpivnitskiy idelpivnitskiy commented Nov 7, 2024

Motivation:

By default we set values for MAX_HEADER_LIST_SIZE and INITIAL_WINDOW_SIZE settings at H2ProtocolConfigBuilder level. However, if users want to change only one setting (for example, MAX_CONCURRENT_STREAMS), they lose our defaults when they use Http2SettingsBuilder.

Modifications:

  • Move defaults initialization from H2ProtocolConfigBuilder to Http2SettingsBuilder.

Result:

Users always start with consistent default Http2Settings and can use builder methods to override what they want.

Motivation:

By default we set values for `MAX_HEADER_LIST_SIZE` and
`INITIAL_WINDOW_SIZE` settings at `H2ProtocolConfigBuilder` level.
However, if users want to change only one setting (for example,
`MAX_CONCURRENT_STREAMS`), they loos our defaults when they use
`Http2SettingsBuilder`.

Modifications:

- Move defaults initialization from `H2ProtocolConfigBuilder` to
`Http2SettingsBuilder`.

Result:

Users always start with consistent default `Http2Settings` and can use
builder methods to override what they want.
@idelpivnitskiy idelpivnitskiy self-assigned this Nov 7, 2024
Copy link
Contributor

@mgodave mgodave left a comment

Choose a reason for hiding this comment

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

This makes sense in the expected behavior that the user would expect.

@idelpivnitskiy idelpivnitskiy merged commit 275623c into apple:main Nov 11, 2024
11 checks passed
@idelpivnitskiy idelpivnitskiy deleted the h2-settings branch November 11, 2024 22:38
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