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

Decrease visibility of Http2FrameCodecBuilder default ctor to protected #11220

Merged
merged 1 commit into from
May 4, 2021

Conversation

idelpivnitskiy
Copy link
Member

@idelpivnitskiy idelpivnitskiy commented May 3, 2021

Motivation:

Http2FrameCodecBuilder defines static factory methods forClient()
and forServer() that should be used to create a new instance.
The default ctor is useful only when users need to override behavior
of the existing builder. Those users should define another way to create
an instance.

Modifications:

  • Decrease visibility of Http2FrameCodecBuilder default ctor from
    public to protected;
  • Add javadoc to clarity responsibilities;

Result:

Users of Http2FrameCodecBuilder are not confused why
new Http2FrameCodecBuilder().build() works for the server-side, but
does not work for the client-side.

Follow-up for #11195.

@idelpivnitskiy idelpivnitskiy changed the title Motivation: Decrease visibility of Http2FrameCodecBuilder default ctor to protected May 4, 2021
…ected`

Motivation:

`Http2FrameCodecBuilder` defines static factory methods `forClient()`
and `forServer()` that should be used to create a new instance.
The default ctor is useful only when users need to override behavior
of the existing builder. Those users should define another way to create
an instance.

Modifications:

- Decrease visibility of `Http2FrameCodecBuilder` default ctor from
`public` to `protected`;
- Add javadoc to clarity responsibilities;

Result:

Users of `Http2FrameCodecBuilder` are not confused why
`new Http2FrameCodecBuilder().build()` works for the server-side, but
does not work for the client-side.
@NiteshKant
Copy link
Member

Users of Http2FrameCodecBuilder are not confused why
new Http2FrameCodecBuilder().build() works for the server-side, but
does not work for the client-side.

@idelpivnitskiy not sure I get this, can you provide a sample code of what does not work?

The intent of the no-arg constructor here is to create a "clean instance" where no state is implicitly set as is the case with the Http2FrameCodecBuilder(boolean server).

@idelpivnitskiy
Copy link
Member Author

For this type of the builder, the isServer flag is a required argument. If users do new Http2FrameCodecBuilder().build(), the default isServer impl will fallback to true, which is correct for the server-side, but may confuse users of the client-side.
It's not easy to use this ctor unless you know how to correctly initialize its state. This is true for users who want to extend the builder, but complicated for direct users of the builder.

@idelpivnitskiy
Copy link
Member Author

The server(boolean) method is not exposed on the builder. Users of the default ctor don't have a way to set this flag unless they extend the builder.

@NiteshKant NiteshKant added this to the 4.1.64.Final milestone May 4, 2021
@idelpivnitskiy idelpivnitskiy merged commit 4010769 into netty:4.1 May 4, 2021
@idelpivnitskiy idelpivnitskiy deleted the Http2FrameCodecBuilder branch May 4, 2021 05:32
normanmaurer pushed a commit that referenced this pull request May 4, 2021
…ected` (#11220)

Motivation:

`Http2FrameCodecBuilder` defines static factory methods `forClient()`
and `forServer()` that should be used to create a new instance.
The default ctor is useful only when users need to override behavior
of the existing builder. Those users should define another way to create
an instance.

Modifications:

- Decrease visibility of `Http2FrameCodecBuilder` default ctor from
`public` to `protected`;
- Add javadoc to clarity responsibilities;

Result:

Users of `Http2FrameCodecBuilder` are not confused why
`new Http2FrameCodecBuilder().build()` works for the server-side, but
does not work for the client-side.
@normanmaurer
Copy link
Member

normanmaurer commented May 4, 2021 via email

raidyue pushed a commit to raidyue/netty that referenced this pull request Jul 8, 2022
…ected` (netty#11220)

Motivation:

`Http2FrameCodecBuilder` defines static factory methods `forClient()`
and `forServer()` that should be used to create a new instance.
The default ctor is useful only when users need to override behavior
of the existing builder. Those users should define another way to create
an instance.

Modifications:

- Decrease visibility of `Http2FrameCodecBuilder` default ctor from
`public` to `protected`;
- Add javadoc to clarity responsibilities;

Result:

Users of `Http2FrameCodecBuilder` are not confused why
`new Http2FrameCodecBuilder().build()` works for the server-side, but
does not work for the client-side.
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.

None yet

3 participants