Hi, Matthias Thanks a lot for joining the discussion.
> Generally, netty upgrades are a source of instability based on past experiences. But considering that we have newer versions (4.1.91.Final) tested with flink-shaded 17.0 in Flink 1.18, it should be alright bumping netty in flink-shaded 16.2 to netty 4.1.83.Final. Agree with your consideration, this also is the reason why we chose 4.1.83.Final. > If no other objections are shared, the next step would be to create a Jira issue, do the change in flink-shaded, release flink-shaded 16.2 and update the flink-shaded version in the 1.17 branch. If there are no objections, I'm ready to create a Jira ticket and prepare the PR for the change. However, it may need assistance from a committer for the subsequent steps. It would be very appreciated if someone could take the releasing task. Thanks Best, Yuxin Matthias Pohl <matthias.p...@aiven.io.invalid> 于2023年10月20日周五 20:30写道： > IHi Yuxin, > That would be the way to go for flink-shaded and Flink 1.17. > > Generally, netty upgrades are a source of instability based on past > experiences. But considering that we have newer versions (4.1.91.Final) > tested with flink-shaded 17.0 in Flink 1.18, it should be alright bumping > netty in flink-shaded 16.2 to netty 4.1.83.Final. > > One argument against such an upgrade would be that the Flink community is > not supporting arm officially (i.e. there's no CI setup). In this sense I > want to wait whether other's have objections against doing the upgrade. > > If no other objections are shared, the next step would be to create a Jira > issue, do the change in flink-shaded, release flink-shaded 16.2 and update > the flink-shaded version in the 1.17 branch. > > Matthias > > On Mon, Oct 16, 2023 at 5:07 AM Yuxin Tan <tanyuxinw...@gmail.com> wrote: > > > Hi, devs, > > > > I would like to revive the discussion again. > > > > In our ARM environment, we encounter a compile error when > > using Flink 1.17. > > > > Flink 1.17 depends on flink-shaded 16.1, which uses netty 4.1.82. > > However, flink-shaded 16.1 fails to compile in the ARM > > environment. As a result, we are unable to compile Flink 1.17 > > due to this issue. > > > > We have tested compiling flink-shaded using netty 4.1.83 or > > a later version in ARM env, and it can compile successfully. > > > > I believe this is a critical bug that needs to be addressed first. > > > > Taking into consideration the previous discussions regarding > > compatibility and the dependency of external connectors on > > this version, I propose addressing the bug by only updating > > flink-shaded's netty to a minor version (e.g., 4.1.83) rather than > > backporting FLINK-32032. This approach can avoid introducing > > other changes, such as updating to a higher netty version (4.1.91), > > potential guava version alterations, Curator version modifications, > > and so on. > > > > WDYT? > > > > > > Best, > > Yuxin > > > > > > Jing Ge <j...@ververica.com.invalid> 于2023年10月5日周四 14:39写道： > > > > > Hi Chesnay, > > > > > > Thanks for joining this discussion and sharing your thoughts! > > > > > > > > > > Connectors shouldn't depend on flink-shaded. > > > > > > > > > > Perfect! We are on the same page. If you could read through the > > discussion, > > > you would realize that, currently, there are many connectors depend on > > > flink-shaded. > > > > > > > > > > Connectors are small enough in scope that depending directly on > > > > guava/jackson/etc. is a fine approach, and they have plenty of other > > > > dependencies that they need to manage anyway; let's treat these the > > same > > > > way. > > > > > > > > > > It is even better, if we could do that. Jira tickest[1] are created. > > > > > > > > > > As for class-loading, there has been a long-standing goal of each > > > > connector being loaded in their own classloader. That still is the > > north > > > > star and the only reasonable way to ensure that multiple connectors > can > > > > be safely used with SQL. > > > > > > > > > > What is the current status? Do we have any Jira ticket for that? > > > > > > Best regards, > > > Jing > > > > > > [1] https://issues.apache.org/jira/browse/FLINK-33190 > > > > > > On Wed, Oct 4, 2023 at 4:43 PM Chesnay Schepler <ches...@apache.org> > > > wrote: > > > > > > > There is no "monolithic" flink-shaded dependency. > > > > Connectors shouldn't depend on anything that Flink provides, but be > > > > self-contained as Martijn pointed out. > > > > > > > > > > > > > > > Connectors shouldn't depend on flink-shaded. > > > > > > > > > > > > > > The overhead and/or risks of doing/supporting that right now far > > > > outweigh the benefits. > > > > ( Because we either have to encode the full version for all > > dependencies > > > > into the package, or accept the risk of minor/patch dependency > clashes) > > > > > > > > > > > > > > Connectors are small enough in scope that depending directly on > > > > guava/jackson/etc. is a fine approach, and they have plenty of other > > > > dependencies that they need to manage anyway; let's treat these the > > same > > > > way. > > > > > > > > > > > > > > Naturally this is also an argument against flink-shaded-connectors; > on > > > > top of that we already experience repo creep and managing releases is > > > > difficult enough as-is. > > > > > > > > > > > > > > > As for class-loading, there has been a long-standing goal of each > > > > connector being loaded in their own classloader. That still is the > > north > > > > star and the only reasonable way to ensure that multiple connectors > can > > > > be safely used with SQL. > > > > > > > > > > > > > > On 02/10/2023 18:32, Jing Ge wrote: > > > > > Hi Sergey, > > > > > > > > > > Thanks for sharing your thoughts. It could somehow help but didn't > > get > > > to > > > > > the root of this issue. > > > > > > > > > > According to the documentation, Flink shaded is used to provide a > > > single > > > > > instance of a shaded dependency across sub-modules in Flink repo. > > > Shaded > > > > > namespaces should be used where shaded dependencies are configured. > > > After > > > > > connectors have been externalized, it ends up with more repos > > depending > > > > on > > > > > one shaded jar, e.g. guava. This is a "monolithic" dependency setup > > > that > > > > > makes it difficult to change the root(flink-shade), because any > > changes > > > > of > > > > > the root have to be propagated to all downstream repos. Even worse > is > > > > that > > > > > not every downstream repo is known while modifying the root. > > > > > > > > > > Since all externalized connectors have their own repos and are not > > > > > sub-modules of Flink anymore, I would suggest the following > upgrade: > > > > > > > > > > 1. Connectors should use their own classloader instead of Flink's > > > > > classloader. This will break the monolithic dependency. Connectors > > and > > > > > Flink can use different versions of flink-shaded. > > > > > 2. [optional] It would be even better that all connector repos > depend > > > on > > > > > their own individual shaded repo, e.g. flink-connector-shaded. > > > > flink-shaded > > > > > should only be used by Flink. > > > > > > > > > > WDYT? > > > > > > > > > > Best regards, > > > > > Jing > > > > > > > > > > > > > > > On Thu, Sep 14, 2023 at 11:28 PM Sergey Nuyanzin < > > snuyan...@gmail.com> > > > > > wrote: > > > > > > > > > >> Yes, that's a reasonable question, thanks for raising it. > > > > >> > > > > >> I think this is not only about flink-shaded, rather about > > dependencies > > > > in > > > > >> general > > > > >> > > > > >> I guess there is no rule of thumb, or at least I'm not aware of > > > > >> Here are my thoughts > > > > >> 1. If bumping dependency doesn't require breaking changes and > passes > > > > >> existing tests then just bump it > > > > >> 2. In case there are breaking changes we could consider doing this > > > > within > > > > >> next major release > > > > >> for minor release > > > > >> a. try to answer a question whether it impacts Flink or not > > > > >> b. in case it impacts Flink and fix itself is relatively > small > > > > then to > > > > >> avoid breaking change > > > > >> we could copy classes with solutions to Flink repo like it > > > > usually > > > > >> happens with Calcite related fixes. > > > > >> The problem of this approach is that I guess it will not > > work > > > > for > > > > >> non jvm deps like e.g. RocksDB > > > > >> c. In case no way to do it without breaking changes for minor > > > > release > > > > >> then probably need sort of announcement motivating to move to > > another > > > > major > > > > >> version where the issue is fixed > > > > >> > > > > >> looking forward to seeing other opinions about that > > > > >> > > > > >> On Wed, Sep 13, 2023 at 9:47 PM Jing Ge > <j...@ververica.com.invalid > > > > > > > >> wrote: > > > > >> > > > > >>> Hi Sergey, > > > > >>> > > > > >>> Thanks for doing the analysis and providing the great insight. I > > did > > > my > > > > >> own > > > > >>> analysis and got the same conclusion. I just wanted to use this > > > example > > > > >> to > > > > >>> kick off a discussion and check if there is a common guideline or > > > > concept > > > > >>> in the community to handle such cases, since it seems any bump-up > > > might > > > > >>> have a big impact. This time, we are kind of lucky. What if CVE > > > related > > > > >>> code has been used in Flink? I do see it is an issue that > > upgrading a > > > > lib > > > > >>> in the flink-shaded repo is not recommended because of the > > > complexity. > > > > >>> IMHO, we don't want to put the cart before the horse. > > > > >>> > > > > >>> Best regards, > > > > >>> Jing > > > > >>> > > > > >>> On Wed, Sep 13, 2023 at 9:11 PM Sergey Nuyanzin < > > snuyan...@gmail.com > > > > > > > > >>> wrote: > > > > >>> > > > > >>>> Thanks for raising this > > > > >>>> > > > > >>>> I would suggest trying to double check whether it actually > impacts > > > > >> Flink > > > > >>> or > > > > >>>> not. > > > > >>>> > > > > >>>> For instance from one side Calcite between 1.22.0..1.31.0 has a > > > > >> critical > > > > >>>> CVE-2022-39135 [1] > > > > >>>> from another side Flink does not use this functionality and is > not > > > > >>> impacted > > > > >>>> by this. > > > > >>>> > > > > >>>> Regarding guava > > > > >>>> After closer look at Guava's high CVE you've mentioned [2] > > > > >>>> Based on Github issue describing the problem (exists since 2016) > > [3] > > > > >>>> there is a security issue with class > > > > >>>> com.google.common.io.FileBackedOutputStream > > > > >>>> > > > > >>>> While looking at source code for > > > > >>>> com.google.common.io.FileBackedOutputStream usage I was not > able > > to > > > > >> find > > > > >>>> such. > > > > >>>> Also I was not able to find usage of > > > > >>>> org.apache.flink.shaded.guava30.com.google.common.io > > > > >> .Files#createTempDir > > > > >>>> which was fixed within commit [4] > > > > >>>> Also I was not able to find other Guava classes which use the > > > problem > > > > >>> one. > > > > >>>> Currently I tend to think that it probably does not impact Flink > > as > > > > >> well. > > > > >>>> Please correct me if I'm wrong > > > > >>>> > > > > >>>> [1] https://nvd.nist.gov/vuln/detail/CVE-2022-39135 > > > > >>>> [2] https://nvd.nist.gov/vuln/detail/CVE-2023-2976 > > > > >>>> [3] https://github.com/google/guava/issues/2575 > > > > >>>> [4] > > > > >>>> > > > > >>>> > > > > >> > > > > > > > > > > https://github.com/google/guava/commit/b3719763b9ca777b94554d7ea2d2b92e27ac6164 > > > > >>>> On Wed, Sep 13, 2023 at 6:26 PM Jing Ge > > <j...@ververica.com.invalid > > > > > > > > >>>> wrote: > > > > >>>> > > > > >>>>> Hi Sergey, Hi Martijn, > > > > >>>>> > > > > >>>>> Thanks for the information and suggestions. It is rational. > > > > >>>>> > > > > >>>>> Since on one hand, we should not do the upgrade according to > all > > > your > > > > >>>>> thoughts, and on the other hand, Guava 30.1.1-jre has known > > CVEs[1] > > > > >> and > > > > >>>> the > > > > >>>>> next closest version that has no CVEs is 32.0.1-jre. Without > the > > > > >>> upgrade, > > > > >>>>> users might not be able to use Flink 1.17 in their production > and > > > > >> Flink > > > > >>>>> 1.18 has not been released yet. Do we have any known concept, > > rule, > > > > >> or > > > > >>>>> process in the community to handle such a common CVE issue? > > > > >>>>> > > > > >>>>> Best regards, > > > > >>>>> Jing > > > > >>>>> > > > > >>>>> > > > > >>>>> > > > > >>>>> [1] > > > > >>> > > https://mvnrepository.com/artifact/com.google.guava/guava/30.1.1-jre > > > > >>>>> On Wed, Sep 13, 2023 at 4:31 PM Martijn Visser < > > > > >>> martijnvis...@apache.org > > > > >>>>> wrote: > > > > >>>>> > > > > >>>>>> Hi Jing, > > > > >>>>>> > > > > >>>>>> Like Sergey said, making that change would break all > connectors > > > > >> that > > > > >>>> used > > > > >>>>>> Flink Shaded in the released versions, given that Guava is > part > > of > > > > >>> the > > > > >>>>>> package name. I think that was the case for Kafka, Pulsar, > JDBC, > > > > >>>> PubSub, > > > > >>>>>> HBase, Cassandra and maybe even more. We shouldn't do this > > > upgrade. > > > > >>>>>> > > > > >>>>>> Best regards, > > > > >>>>>> > > > > >>>>>> Martijn > > > > >>>>>> > > > > >>>>>> On Wed, Sep 13, 2023 at 2:54 PM Sergey Nuyanzin < > > > > >> snuyan...@gmail.com > > > > >>>>>> wrote: > > > > >>>>>> > > > > >>>>>>> Hi Jing, > > > > >>>>>>> > > > > >>>>>>> If I remember correctly, bumping of guava to another major > > > > >> version > > > > >>>>>> usually > > > > >>>>>>> leads to package rename > > > > >>>>>>> since the major version number is a part of the package > name... > > > > >>>>>>> Not 100% sure however this could be the reason for some > > potential > > > > >>>>>> breaking > > > > >>>>>>> changes (we also faced that with connectors while the last > > > > >>>> flink-shaded > > > > >>>>>>> update) > > > > >>>>>>> > > > > >>>>>>> On Wed, Sep 13, 2023 at 2:43 PM Jing Ge > > > > >> <j...@ververica.com.invalid > > > > >>>>>>> wrote: > > > > >>>>>>> > > > > >>>>>>>> Hi Martijn, > > > > >>>>>>>> > > > > >>>>>>>> For example, bump up guava from 30.1.1-jre to 32.1.2-jre. > > > > >>>>>>>> > > > > >>>>>>>> Best regards, > > > > >>>>>>>> Jing > > > > >>>>>>>> > > > > >>>>>>>> On Wed, Sep 13, 2023 at 2:37 PM Martijn Visser < > > > > >>>>>> martijnvis...@apache.org > > > > >>>>>>>> wrote: > > > > >>>>>>>> > > > > >>>>>>>>> Hi Jing, > > > > >>>>>>>>> > > > > >>>>>>>>> A PR to update the readme sounds like a good plan. > > > > >>>>>>>>> > > > > >>>>>>>>> I think it depends on what are the expected updates for a > > > > >>>>>> flink-shaded > > > > >>>>>>>>> 16.2, since that version doesn't exist. > > > > >>>>>>>>> > > > > >>>>>>>>> Best regards, > > > > >>>>>>>>> > > > > >>>>>>>>> Martijn > > > > >>>>>>>>> > > > > >>>>>>>>> On Wed, Sep 13, 2023 at 1:48 PM Jing Ge > > > > >>>> <j...@ververica.com.invalid > > > > >>>>>>>>> wrote: > > > > >>>>>>>>> > > > > >>>>>>>>>> Hi Martijn, > > > > >>>>>>>>>> > > > > >>>>>>>>>> Thanks for your reply with details. Appreciate it. > > > > >>>>>>>>>> > > > > >>>>>>>>>> > > > > >>>>>>>>>>> Flink-Shaded is usually only updated whenever a > > > > >>>>>>>>>>> new Flink minor version is released and only at the > > > > >>> beginning > > > > >>>>> of > > > > >>>>>>> the > > > > >>>>>>>>>>> release cycle, so that there's enough time to stabilize > > > > >>>> Flink. > > > > >>>>>>>>>> > > > > >>>>>>>>>> This is the information I am looking for. It will help > devs > > > > >>>>>>> understand > > > > >>>>>>>>> the > > > > >>>>>>>>>> compatibility between different versions of flink and > > > > >>>>> flink-shaded, > > > > >>>>>>> if > > > > >>>>>>>> it > > > > >>>>>>>>>> could be described in the readme. If you don't mind, I can > > > > >>>>> create a > > > > >>>>>>> pr > > > > >>>>>>>>> and > > > > >>>>>>>>>> update it. > > > > >>>>>>>>>> > > > > >>>>>>>>>> Speaking of this rule, I have a follow-up question: do you > > > > >>> have > > > > >>>>> any > > > > >>>>>>>>> concern > > > > >>>>>>>>>> if flink-shaded 16.2 will be released and upgraded(from > > > > >> 16.1 > > > > >>> to > > > > >>>>>> 16.2) > > > > >>>>>>>> in > > > > >>>>>>>>>> Flink 1.17? Is there anything we should pay attention to > > > > >>> while > > > > >>>>>>>> releasing > > > > >>>>>>>>> a > > > > >>>>>>>>>> new minor flink-shaded version? > > > > >>>>>>>>>> > > > > >>>>>>>>>> Best regards, > > > > >>>>>>>>>> Jing > > > > >>>>>>>>>> > > > > >>>>>>>>>> On Wed, Sep 13, 2023 at 9:01 AM Martijn Visser < > > > > >>>>>>>> martijnvis...@apache.org > > > > >>>>>>>>>> wrote: > > > > >>>>>>>>>> > > > > >>>>>>>>>>> Hi Jing, > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> Flink Shaded exists so that Flinks internal usage of > > > > >>> commonly > > > > >>>>>> used > > > > >>>>>>>>>> packages > > > > >>>>>>>>>>> such as Guava, Jackson inside of Flink don't clash with > > > > >>>>> different > > > > >>>>>>>>>> versions > > > > >>>>>>>>>>> that users might use when creating a Flink application. > > > > >>> When > > > > >>>> I > > > > >>>>>> did > > > > >>>>>>>> the > > > > >>>>>>>>>>> upgrade of Flink Shaded, we already ran into a bunch of > > > > >>>>> problems > > > > >>>>>>>>> because > > > > >>>>>>>>>> a > > > > >>>>>>>>>>> lot of the externalized connectors relied on Flink > > > > >> Shaded, > > > > >>>>> which > > > > >>>>>>> made > > > > >>>>>>>>> it > > > > >>>>>>>>>>> problematic to get the connector to work on both Flink > > > > >> 1.17 > > > > >>>> and > > > > >>>>>>> Flink > > > > >>>>>>>>>> 1.18. > > > > >>>>>>>>>>> There's been quite a lot of effort put into making sure > > > > >>> that > > > > >>>>>>>>> externalized > > > > >>>>>>>>>>> connectors don't rely on Flink Shaded at all anymore, by > > > > >>>> either > > > > >>>>>>> using > > > > >>>>>>>>>> their > > > > >>>>>>>>>>> own versions of shaded artifacts (which was the case with > > > > >>> the > > > > >>>>>>> Pulsar > > > > >>>>>>>>>>> connector) or just removing the dependency on Flink > > > > >> Shaded > > > > >>>> all > > > > >>>>>>>>> together, > > > > >>>>>>>>>> by > > > > >>>>>>>>>>> using regular Java. > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> If you would upgrade flink-shaded from 16.1 to 17.0 in > > > > >>> Flink > > > > >>>>>> 1.17, > > > > >>>>>>>> you > > > > >>>>>>>>>>> would break all externalized connectors that rely on > > > > >> Flink > > > > >>>>>> Shaded's > > > > >>>>>>>>> Guava > > > > >>>>>>>>>>> version, plus you potentially would impact the runtime > > > > >>> given > > > > >>>>> that > > > > >>>>>>>>>> there's a > > > > >>>>>>>>>>> newer Netty version etc. Flink-Shaded is usually only > > > > >>> updated > > > > >>>>>>>> whenever > > > > >>>>>>>>> a > > > > >>>>>>>>>> new Flink minor version is released and only at the > > > > >> beginning > > > > >>>> of > > > > >>>>>> the > > > > >>>>>>>>>>> release cycle, so that there's enough time to stabilize > > > > >>>> Flink. > > > > >>>>>>>>>> All in all, we shouldn't upgrade flink-shaded in Flink > > > > >> 1.17. > > > > >>>>>>>>>>> Best regards, > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> Martijn > > > > >>>>>>>>>>> > > > > >>>>>>>>>>> On Tue, Sep 12, 2023 at 7:26 PM Jing Ge > > > > >>>>>> <j...@ververica.com.invalid > > > > >>>>>>>>>>> wrote: > > > > >>>>>>>>>>> > > > > >>>>>>>>>>>> Hi Dev, > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> Currently Flink 1.17 is using flink-shaded 16.1 and > > > > >> Flink > > > > >>>>> 1.18 > > > > >>>>>> is > > > > >>>>>>>>> using > > > > >>>>>>>>>>>> flink-shaded 17.0. Do we need to consider any > > > > >>> compatibility > > > > >>>>>> rules > > > > >>>>>>>>>> between > > > > >>>>>>>>>>>> them? E.g. is there any concern to upgrade flink-shaded > > > > >>>> from > > > > >>>>>> 16.1 > > > > >>>>>>>> to > > > > >>>>>>>>>> 17.x > > > > >>>>>>>>>>>> for Flink 1.17? Or there are some implicit dependency > > > > >>> rules > > > > >>>>>>> between > > > > >>>>>>>>>>>> them. Looking > > > > >>>>>>>>>>>> forward to hearing from you. > > > > >>>>>>>>>>>> > > > > >>>>>>>>>>>> Best regards, > > > > >>>>>>>>>>>> Jing > > > > >>>>>>>>>>>> > > > > >>>>>>> > > > > >>>>>>> -- > > > > >>>>>>> Best regards, > > > > >>>>>>> Sergey > > > > >>>>>>> > > > > >>>> > > > > >>>> -- > > > > >>>> Best regards, > > > > >>>> Sergey > > > > >>>> > > > > >> > > > > >> -- > > > > >> Best regards, > > > > >> Sergey > > > > >> > > > > > > > > > > > > > >
