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 frameType method to Http2Frame. #13989

Open
wants to merge 1 commit into
base: 4.2
Choose a base branch
from
Open

Conversation

He-Pin
Copy link
Contributor

@He-Pin He-Pin commented Apr 21, 2024

Motivation:

Add frameType to Http2Frame, so I can use it more easily with switch expression than the current instance of.

Modification:

Add frameType to Http2Frame.

Result:

Easier to work with in Java < 17

@He-Pin He-Pin force-pushed the frameTypes branch 2 times, most recently from 445dd3f to 6ce9758 Compare April 21, 2024 06:56
@He-Pin
Copy link
Contributor Author

He-Pin commented Apr 21, 2024

Cause:

Error: Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.15.4:cmp (default) on project netty-codec-http2: There is at least one incompatibility: io.netty.handler.codec.http2.AbstractHttp2StreamFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2DataFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2Frame.frameType():METHOD_ADDED_TO_INTERFACE,io.netty.handler.codec.http2.Http2GoAwayFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2HeadersFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2PingFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2PriorityFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2PushPromiseFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2ResetFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2SettingsAckFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2SettingsFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2StreamFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2UnknownFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE,io.netty.handler.codec.http2.Http2WindowUpdateFrame:METHOD_ABSTRACT_ADDED_IN_IMPLEMENTED_INTERFACE -> [Help 1]

            <excludes>
              <exclude>@io.netty.util.internal.UnstableApi</exclude>
              <exclude>io.netty.util.internal.shaded</exclude>
            </excludes>

it is marked with annotation, but still does not pass the japicmp check. strange.

@He-Pin
Copy link
Contributor Author

He-Pin commented Apr 21, 2024

Error: Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.20.0:cmp (default) on project netty-buffer: There is at least one incompatibility: io.netty.buffer.Unpooled.copiedBuffer(byte[]):METHOD_NO_LONGER_VARARGS,io.netty.buffer.Unpooled.wrappedBuffer(byte[]):METHOD_NO_LONGER_VARARGS -> [Help 1]

reported : https://github.com/siom79/japicmp/issues

So another new issue found after upgrade the japicmp seems like an issue in japicmp

@normanmaurer
Copy link
Member

I don't think k this is an acceptable breakage for 4.1

@He-Pin
Copy link
Contributor Author

He-Pin commented Apr 21, 2024

Still has :

Error:  Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.15.7:cmp (default) on project netty-codec-http2: There is at least one incompatibility: io.netty.handler.codec.http2.Http2Frame.frameType():METHOD_ADDED_TO_INTERFACE

:(

@He-Pin
Copy link
Contributor Author

He-Pin commented Apr 21, 2024

@normanmaurer thanks, I made this change because I saw it's marked with @UnstableApi
I'm planning migrate our internal jetty based spdy services to Netty's http/2 or http/3 one

@normanmaurer
Copy link
Member

While it's marked as unstable I still feel not comfortable doing the changes as the benefit is not high enough vs maybe breaking people

@He-Pin
Copy link
Contributor Author

He-Pin commented Apr 21, 2024

I have to agree with you on this.

btw: I checked the Http2FrameCodec, and the Http3RequestStreamInboundHandler:
In Http2FrameCodec Netty following the on*Read style ,eg onDataRead/onGoAway read, but in the Http3RequestStreamInboundHandler IIRC, Netty is following channelRead() with overloaded methods style, is there any particular reason for the new design?

@He-Pin
Copy link
Contributor Author

He-Pin commented Apr 22, 2024

If there is a Netty 4.2.x release, then this should not be a problem with Java 8:)

@He-Pin He-Pin changed the base branch from 4.1 to 4.2 April 23, 2024 02:40
*/
default byte frameType() {
throw new NotImplementedYetException();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think with Java 8's default method, this will not cause any problem.

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

2 participants