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

Migrate the classic transport to Netty 4 without CVEs #643

Merged
merged 5 commits into from
Sep 15, 2023

Conversation

He-Pin
Copy link
Member

@He-Pin He-Pin commented Sep 9, 2023

Motivation:
Better performance and no CVES.

Open for votes.

refs: https://issues.apache.org/jira/browse/FLINK-29065
was: #540
This change simpler vs the previous one.

refs: apache/flink#22271
refs: apache/flink#22996

NOTE:
Need to make sure MultiJvm/test pass.

And Netty 4's migration guide: https://github.com/netty/netty/wiki/New-and-noteworthy-in-4.0

image
image

@zentol cc

@He-Pin He-Pin marked this pull request as draft September 9, 2023 11:01
@He-Pin He-Pin force-pushed the classicNetty4 branch 3 times, most recently from 781f07d to 2517d0a Compare September 9, 2023 13:17
Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

I know we wanted to do a community review on this but since its a draft it cannot be merged anyways.

Also considering the comparitive triviality of this PR, I don't think there is any real harm in merging this. Nothing is stopping us from dropping classical remoting later if we decide to do so, but the fact still remains that either we ship with CVE's or break SemVer

project/Dependencies.scala Outdated Show resolved Hide resolved
@He-Pin He-Pin added this to the 1.1.0 milestone Sep 11, 2023
@He-Pin
Copy link
Member Author

He-Pin commented Sep 12, 2023

I think I had ran 10+ tests and all passed.

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

LGTM. Cool!

Copy link
Contributor

@mdedetrich mdedetrich left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@pjfanning pjfanning left a comment

Choose a reason for hiding this comment

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

lgtm

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.

4 participants