Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add example for h2 keep alive #2203
Add example for h2 keep alive #2203
Changes from all commits
a99463b
f240285
0bbc808
b78c5d3
fdd8b70
f452fb1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The improved comments will really help.
h2().keepAlivePolicy(new KeepAlivePolicyBuilder().build())
is needed to configure the default. Perhaps a constant
defaultKeepAlive()
oruseDefaults()
for configuring the default.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we expose the builder type I don't think the static factory helper methods provide much value and add duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, but I had to spend a couple minutes figuring out that there was no default object and how to make one (the Builder docs don't mention that default values will be use if not supplied). Oddly, we provide a constant for disabling keep alive which is the default behaviour (not clear why you would ever needed it except to be explicit). Some mechanism or doc suggestion assistance of how to get a default keep alive policy would help IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets consider this for a followup PR to make any changes more explicit. I would prefer to remove the
disabled()
API and just usenull
if we need a default (one less API, no special "by reference" comparison required).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All are fine approaches, whatever ensures users don't have to read the source to figure out how to get the default.