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

Add new pipeline helpers and deprecate old paths #226

Merged
merged 1 commit into from
Aug 4, 2020

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Aug 4, 2020

Motivation:

As part of #214 we need to introduce new pipeline helpers to configure
the channel pipeline with the new handlers. This PR adds those helpers
and deprecates the old paths.

Modifications:

  • Add pipeline helpers with the new streamID-less initializer
  • Switch old pipeline helpers which provided codecs to use the new path
    and codecs
  • Duplicate SimpleClientServerTests and ConfiguringPipelineTests to add
    frame payload based counterparts
  • Deprecate older helpers and multiplexer init, delete tests which don't
    hit deprecated paths (note: these still exist in the payload based
    counterparts)
  • Update Bench1Conn10kRequests to be payload based

Result:

  • More 'FramePayload' based Tests
  • Paths for creating an 'HTTP2Frame' based stream via
    'HTTP2StreamMultiplexer' are now deprecated

@glbrntt glbrntt added the 🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0 label Aug 4, 2020
@glbrntt glbrntt requested a review from Lukasa August 4, 2020 10:23
@glbrntt
Copy link
Contributor Author

glbrntt commented Aug 4, 2020

Oops. Looks like I missed some tests.

Motivation:

As part of apple#214 we need to introduce new pipeline helpers to configure
the channel pipeline with the new handlers. This PR adds those helpers
and deprecates the old paths.

Modifications:

- Add pipeline helpers with the new streamID-less initializer
- Switch old pipeline helpers which provided codecs to use the new path
  and codecs
- Duplicate SimpleClientServerTests and ConfiguringPipelineTests to add
  frame payload based counterparts
- Deprecate older helpers and multiplexer init, delete tests which don't
  hit deprecated paths (note: these still exist in the payload based
  counterparts)
- Update Bench1Conn10kRequests to be payload based

Result:

- More 'FramePayload' based Tests
- Paths for creating an 'HTTP2Frame' based stream via
  'HTTP2StreamMultiplexer' are now deprecated
@glbrntt glbrntt force-pushed the gb-deprecate-frame-based-streams branch from 9bf21be to cca5708 Compare August 4, 2020 10:34
@glbrntt
Copy link
Contributor Author

glbrntt commented Aug 4, 2020

The API breakage is expected: we changed the argument name and removed the default value of an init which is yet to be released.

11:35:34 Constructor HTTP2StreamMultiplexer.init(mode:channel:targetWindowSize:outboundBufferSizeHighWatermark:outboundBufferSizeLowWatermark:inboundStreamStateInitializer:) has parameter 5 type change from ((NIO.Channel) -> NIO.EventLoopFuture<Swift.Void>)? to ((NIO.Channel, NIOHTTP2.HTTP2StreamID) -> NIO.EventLoopFuture<Swift.Void>)?
11:35:34 Constructor HTTP2StreamMultiplexer.init(mode:channel:targetWindowSize:outboundBufferSizeHighWatermark:outboundBufferSizeLowWatermark:inboundStreamStateInitializer:) has removed default argument from parameter 3
11:35:34 Constructor HTTP2StreamMultiplexer.init(mode:channel:targetWindowSize:outboundBufferSizeHighWatermark:outboundBufferSizeLowWatermark:inboundStreamStateInitializer:) has removed default argument from parameter 4

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Nice, seems good to me!

@Lukasa Lukasa merged commit 177c50a into apple:master Aug 4, 2020
@glbrntt glbrntt deleted the gb-deprecate-frame-based-streams branch August 4, 2020 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔼 needs-minor-version-bump For PRs that when merged cause a bump of the minor version, ie. 1.x.0 -> 1.(x+1).0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants