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

Override default maxConcurrentStreams when > 0 #2165

Merged
merged 1 commit into from Aug 24, 2018

Conversation

Projects
None yet
5 participants
@chbatey
Member

chbatey commented Aug 22, 2018

  • Sets parallelism = maxConcurrentStreams, otherwise it goes through as 0
  • Fix when default from config is applied
Override default maxConcurrentStreams when 0
* Sets parallism = maxConcurrentStreams, otherwise it goes through as 0
* Fix when default from config is applied

@chbatey chbatey requested review from raboof and ktoso Aug 22, 2018

@akka-ci akka-ci added the validating label Aug 22, 2018

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Aug 22, 2018

Member

Oh man, that was sloppy. Thanks for catching that...

Member

jrudolph commented Aug 22, 2018

Oh man, that was sloppy. Thanks for catching that...

@akka-ci akka-ci added tested and removed validating labels Aug 22, 2018

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Aug 22, 2018

Collaborator

Test PASSed.

Collaborator

akka-ci commented Aug 22, 2018

Test PASSed.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Aug 22, 2018

Member

I created #2168 to track the regression. Maybe you can add a note about what the symptoms are when using the default parallelism=0.

Member

jrudolph commented Aug 22, 2018

I created #2168 to track the regression. Maybe you can add a note about what the symptoms are when using the default parallelism=0.

@jrudolph

LGTM, I'll create a ticket for follow ups.

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Aug 22, 2018

Member

#2170 to add end-to-end testing for HTTP/2.

Member

jrudolph commented Aug 22, 2018

#2170 to add end-to-end testing for HTTP/2.

else settings.mapHttp2Settings(_.withMaxConcurrentStreams(parallelism))
Http2Shadow.bindAndHandleAsync(handler, interface, port, connectionContext, definitiveSettings, parallelism, log)(fm)
else settings
Http2Shadow.bindAndHandleAsync(handler, interface, port, connectionContext, definitiveSettings, definitiveSettings.http2Settings.maxConcurrentStreams, log)(fm)

This comment has been minimized.

@raboof

raboof Aug 23, 2018

Member

If it is always the case that parallelism == definitiveSettings.http2Settings.maxConcurrentStreams, shouldn't we remove the parallelism parameter?

@raboof

raboof Aug 23, 2018

Member

If it is always the case that parallelism == definitiveSettings.http2Settings.maxConcurrentStreams, shouldn't we remove the parallelism parameter?

This comment has been minimized.

@jrudolph

jrudolph Aug 23, 2018

Member

Yes. The general idea is to move away from parameters to config where possible.

I'll create a ticket.

@jrudolph

jrudolph Aug 23, 2018

Member

Yes. The general idea is to move away from parameters to config where possible.

I'll create a ticket.

This comment has been minimized.

@ktoso

ktoso Aug 24, 2018

Member

Sounds like we can do this in separate PR and merge this fix for now then

@ktoso

ktoso Aug 24, 2018

Member

Sounds like we can do this in separate PR and merge this fix for now then

@ktoso

ktoso approved these changes Aug 24, 2018

Whoops, silly bug...

@ktoso ktoso merged commit 7564e5d into akka:master Aug 24, 2018

3 checks passed

Jenkins PR Validation Test PASSed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
typesafe-cla-validator All users have signed the CLA
Details

@ktoso ktoso added this to the 10.1.5 milestone Aug 24, 2018

@ktoso ktoso changed the title from Override default maxConcurrentStreams when 0 to Override default maxConcurrentStreams when > 0 Aug 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment