[FLINK-34234] Bumping version of flatten plugin to 1.2.7 #136

snuyanzin · 2024-01-25T08:53:48Z

~~As a test [1] there was first a commit with enabling ShadeOptionalChecker however without adding optional. As a result it detects the issue i a number of places.~~

UPD: Since bumping version of flatten plugin helps I reverted changes (to not force push) and added version bump itself to 1.2.7
also keep a couple of hotfix commits as a result of review

snuyanzin · 2024-01-25T12:17:49Z

.github/workflows/ci.yml

      - name: Check licensing
        run: |
-          mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \


align with naming in pom

That might deserve it's own hotfix commit

yep, i was thinking about that however had some doubts...
Since you've also mentioned this I think we should do that

extracted into a separate commit

flink-shaded-asm-9/pom.xml

pom.xml

XComp

Thanks for your help, @snuyanzin. I verified that 7caf8cd which the failed build is based on matches the changes of this PR's first commit ab87bb4 The changes look good to me overall. I have a few comments. PTAL

XComp · 2024-01-25T12:27:21Z

.github/workflows/ci.yml

      - name: Check licensing
        run: |
-          mvn ${MVN_COMMON_OPTIONS} exec:java@check-licensing -N \


That might deserve it's own hotfix commit

pom.xml

.github/workflows/ci.yml

XComp · 2024-01-25T12:39:43Z

flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml

@@ -128,6 +131,7 @@ under the License.
                <groupId>org.apache.zookeeper</groupId>
                <artifactId>zookeeper</artifactId>
                <version>${zookeeper.version}</version>
+                <optional>${flink.markBundledAsOptional}</optional>
            </dependency>
            <dependency>
                <groupId>io.netty</groupId>


we don't need that for BOMs?!

frankly speaking I don't know
I will see whether it will work without this (together with Flink tests)

That's not about BOMs
in general it does not make sense to add it for depedencies inside dependencyManagement
I removed it for others in dependencyManagement, thanks for noticing this 👍

Can you elaborate a bit here? It doesn't make sense because dependencies from the <dependencyManagement/> section would need to be explicitly added to a module pom to make the dependency available and only then would we have to add the <optional/> tag?!

Looks like we have 4 locations in flink-dist/pom.xml and flink-filesystems/flink-s3-fs-presto/pom.xml where this also applies. 🤔

May be I was not clear enough, sorry
By saying

it does not make sense to add it for depedencies inside dependencyManagement

I ment not in general. Just for the case here
Here there is already existing dependency on zookeeper for every zookeeper module in dependencies which overrides the one from dependency management (optional tag).
Netty here comes only as a dependency from zookeeper and once zookeeper is marked optional maven started to think that netty is also becoming optional
I don't think this is the case for poms you've mentioned

XComp · 2024-01-25T12:47:01Z

pom.xml

@@ -65,6 +65,7 @@ under the License.
        <netty.version>4.1.100.Final</netty.version>
        <jackson.version>2.15.3</jackson.version>
        <guava.version>32.1.3-jre</guava.version>
+        <flink.markBundledAsOptional>true</flink.markBundledAsOptional>


doesn't it make sense to upgrade the shade plugin to 3.4.1 here as well instead of having it only being upgraded for the jackson dependencies?

my understanding is (according to your comment in FLINK-34148) that the shade plugin stopped editing the dependency tree in 3.4.1.

Maven 3.3+ didn't allow editing the dependency tree anymore (based on the description in FLINK-28016). Shouldn't we have run into issues already earlier because we upgraded Maven in CI to 3.8.2 even though we were still using an older version of the shade plugin? It seems that I'm missing something here. 🤔

If I understand things around MNG-5899 correctly in maven they stopped reuse dependency reduced pom produced by shade plugin however they didn't forbid manipulation on api level.
At the same time on api level it is still possible to get some info e.g. list of dependencies from maven and then make some manipulation in code with this list of dependencies, this is how it was in maven-shade-plugin [1]. After MSHADE-413 instead of manipulation on collection of objects received from maven they started to clone it first and then perform same manipulation on cloned version

[1] https://github.com/apache/maven-shade-plugin/pull/124/files#diff-083ef87367e79a2ea6b333846149588f4ce284ed930db1d87712fdbf9111e381L1066-L1112

I see, those were different issue. But that still leaves the question open whether we want to enable the more recent shade plugin version for all modules. 🤔

that was the plan
the only thing stopped me from making it here is that I was thinking about making ShadeOptionalChecker working which then could be backported to 18.x and then as a separate task sync maven-shade-plugin for all the modules

Or do you think it worth making it here?

fair point 👍 a follow-up Jira task is fine as well.

yep, sure FLINK-34269

snuyanzin · 2024-01-26T09:11:34Z

force-pushed since have to extract license related update into a separate commit

.github/workflows/ci.yml

XComp · 2024-01-26T13:48:09Z

.github/workflows/ci.yml

+      - name: Check shade optional dependencies
+        run: |
+            mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}


Suggested change

mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check | tee ${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}

mvn ${MVN_COMMON_OPTIONS} dependency:tree | tee ${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}

The profile is not necessary for the dependency:tree IIUC.

In fact it is...
The reason is that version suffix of several modules like guava, zookeeper is added only with this profile e.g. [1] and we need to know module name together with version suffix while parsing output of dependency:tree

flink-shaded/flink-shaded-guava-32/pom.xml

Lines 52 to 59 in b4f9ed2

<profiles>

<profile>

<id>license-check</id>

<properties>

<flink.ci.license.suffix>-32</flink.ci.license.suffix>

</properties>

</profile>

</profiles>

may be we need to rename the profile itself... 🤔

<!-- The license check and the optional shade check require the artifactId to match the directory that the module resides in.

hm, ok, if that's the case we could update the comment in the parent pom file to also mention the shade checker

btw. the exec-maven-plugin allows to set System properties (see docs) which could be used to enable profiles implicitly (see docs). 😇

XComp · 2024-01-26T14:41:32Z

flink-shaded-zookeeper-parent/flink-shaded-zookeeper-38/pom.xml

@@ -128,6 +131,7 @@ under the License.
                <groupId>org.apache.zookeeper</groupId>
                <artifactId>zookeeper</artifactId>
                <version>${zookeeper.version}</version>
+                <optional>${flink.markBundledAsOptional}</optional>
            </dependency>
            <dependency>
                <groupId>io.netty</groupId>


Can you elaborate a bit here? It doesn't make sense because dependencies from the <dependencyManagement/> section would need to be explicitly added to a module pom to make the dependency available and only then would we have to add the <optional/> tag?!

Looks like we have 4 locations in flink-dist/pom.xml and flink-filesystems/flink-s3-fs-presto/pom.xml where this also applies. 🤔

XComp · 2024-01-26T14:53:48Z

pom.xml

@@ -65,6 +65,7 @@ under the License.
        <netty.version>4.1.100.Final</netty.version>
        <jackson.version>2.15.3</jackson.version>
        <guava.version>32.1.3-jre</guava.version>
+        <flink.markBundledAsOptional>true</flink.markBundledAsOptional>


I see, those were different issue. But that still leaves the question open whether we want to enable the more recent shade plugin version for all modules. 🤔

zentol

Please see my comments in the ticket before continuing with this PR.

snuyanzin · 2024-01-30T11:48:24Z

Thanks for your feedback @zentol
I checked locally with 1.2.7 flatten plugin and confirm that it resolves the issue

So I bumped the issue and currently reverted previous changes regarding shading...

May be one question which is still not clear to me (not directly related to perf regression)
Could you please clarify:
In release's profile[1] for flink-shaded it is mentioned that we need to use maven < 3.3 to release.
Playing locally I was not able to find a difference between the result of usage 3.2.5 and 3.8.6 (for both the perf regression issue is reproduced in case of flatten plugin 1.2.1 and for both it is fixed with 1.2.7).
Do we need still apply ShadeOptionalChecker's approach like in Flink main repo to safely use 3.8.6 during release?

[1]

flink-shaded/pom.xml

Lines 136 to 137 in b4f9ed2

    
           <!-- maven version must be lower than 3.3. See FLINK-3158 --> 
        
           <version>(,3.3)</version>

zentol · 2024-06-27T11:22:21Z

was not able to find a difference

That's because flink-shaded modules are all self-contained; no flink-shaded module depends on another flink-shaded module, due to which right now it's not affected by the problem we had in the main repo.

This means you can probably use newer maven versions, but still shouldn't until we have added safeguards that either no interdependencies between modules exist or everything is marked as optional.
But I'd treat that as a separate issue altogether.

XComp

LGTM 👍 I verified that 1.2.7 is the newest 1.2.x patch version

snuyanzin force-pushed the optional2 branch from 166a436 to 73a7ffa Compare January 25, 2024 12:14

snuyanzin changed the title ~~Enable check for shading optional~~ [FLINK-34234] Apply ShadeOptionalChecker for flink-shaded Jan 25, 2024

snuyanzin marked this pull request as ready for review January 25, 2024 12:15

snuyanzin commented Jan 25, 2024

View reviewed changes

flink-shaded-asm-9/pom.xml Outdated Show resolved Hide resolved

snuyanzin commented Jan 25, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

snuyanzin requested a review from XComp January 25, 2024 12:19

XComp requested changes Jan 25, 2024

View reviewed changes

[hotfix] Move LicenseChecker configuration under execution

08ebf27

snuyanzin force-pushed the optional2 branch from 73a7ffa to 874db1e Compare January 26, 2024 09:04

XComp reviewed Jan 26, 2024

View reviewed changes

zentol reviewed Jan 30, 2024

View reviewed changes

snuyanzin changed the title ~~[FLINK-34234] Apply ShadeOptionalChecker for flink-shaded~~ [FLINK-34234] Bumping version of flatten plugin to 1.2.7 Jun 27, 2024

snuyanzin requested a review from XComp June 27, 2024 11:31

[FLINK-34234] Enable check for shading optiona

1bf31a8

snuyanzin force-pushed the optional2 branch from a695921 to 1bf31a8 Compare June 27, 2024 12:11

XComp approved these changes Jun 27, 2024

View reviewed changes

snuyanzin merged commit 26473c0 into apache:master Jun 27, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-34234] Bumping version of flatten plugin to 1.2.7 #136

[FLINK-34234] Bumping version of flatten plugin to 1.2.7 #136

snuyanzin commented Jan 25, 2024 •

edited

Loading

snuyanzin Jan 25, 2024

XComp Jan 25, 2024

snuyanzin Jan 26, 2024

snuyanzin Jan 26, 2024

XComp left a comment •

edited

Loading

XComp Jan 25, 2024

XComp Jan 25, 2024

snuyanzin Jan 25, 2024

snuyanzin Jan 26, 2024 •

edited

Loading

XComp Jan 26, 2024

snuyanzin Jan 26, 2024 •

edited

Loading

XComp Jan 25, 2024

snuyanzin Jan 25, 2024

XComp Jan 26, 2024

snuyanzin Jan 26, 2024

XComp Jan 29, 2024

snuyanzin Jan 30, 2024

snuyanzin commented Jan 26, 2024 •

edited

Loading

XComp Jan 26, 2024

snuyanzin Jan 26, 2024

snuyanzin Jan 26, 2024

XComp Jan 26, 2024

XComp Jan 26, 2024

XComp Jan 26, 2024

XComp Jan 26, 2024

zentol left a comment

snuyanzin commented Jan 30, 2024 •

edited

Loading

zentol commented Jun 27, 2024

XComp left a comment

	mvn ${MVN_COMMON_OPTIONS} dependency:tree -Plicense-check \| tee ${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}
	mvn ${MVN_COMMON_OPTIONS} dependency:tree \| tee ${{ env.MVN_DEPENDENCY_PLUGIN_OUTPUT_FILE }}

	<profiles>
	<profile>
	<id>license-check</id>
	<properties>
	<flink.ci.license.suffix>-32</flink.ci.license.suffix>
	</properties>
	</profile>
	</profiles>

[FLINK-34234] Bumping version of flatten plugin to 1.2.7 #136

[FLINK-34234] Bumping version of flatten plugin to 1.2.7 #136

Conversation

snuyanzin commented Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XComp left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snuyanzin Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snuyanzin Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snuyanzin commented Jan 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zentol left a comment

Choose a reason for hiding this comment

snuyanzin commented Jan 30, 2024 • edited Loading

zentol commented Jun 27, 2024

XComp left a comment

Choose a reason for hiding this comment

snuyanzin commented Jan 25, 2024 •

edited

Loading

XComp left a comment •

edited

Loading

snuyanzin Jan 26, 2024 •

edited

Loading

snuyanzin Jan 26, 2024 •

edited

Loading

snuyanzin commented Jan 26, 2024 •

edited

Loading

snuyanzin commented Jan 30, 2024 •

edited

Loading