[AMORO-2113] Apply spotless-maven-plugin in trino module #2131

HuangFru · 2023-10-17T06:55:16Z

Why are the changes needed?

Close #2113.

Brief change log

Apply spotless-maven-plugin in trino module.
Upgrade googleJavaFormat to 1.15.0 only to the Trino module.
Skip maven-checkstyle-plugin.
mvn spotless:apply -pl trino
Skip trino module by default in Maven.

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before making a pull request

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (not applicable)

This reverts commit 7415823.

codecov · 2023-10-17T07:21:31Z

Codecov Report

Attention: 352 lines in your changes are missing coverage. Please review.

Files	Coverage Δ
...in/java/com/netease/arctic/trino/ArcticConfig.java	`68.42% <ø> (ø)`
...java/com/netease/arctic/trino/ArcticConnector.java	`100.00% <100.00%> (ø)`
...m/netease/arctic/trino/ArcticConnectorFactory.java	`100.00% <100.00%> (ø)`
...ease/arctic/trino/ArcticConnectorSplitManager.java	`100.00% <100.00%> (ø)`
...tease/arctic/trino/ArcticHadoopAuthentication.java	`0.00% <ø> (ø)`
...netease/arctic/trino/ArcticHdfsAuthentication.java	`72.72% <100.00%> (+2.72%)`	⬆️
.../netease/arctic/trino/ArcticHdfsConfiguration.java	`100.00% <100.00%> (ø)`
...om/netease/arctic/trino/ArcticMetadataFactory.java	`100.00% <100.00%> (ø)`
...in/java/com/netease/arctic/trino/ArcticModule.java	`100.00% <100.00%> (ø)`
...netease/arctic/trino/ArcticPageSourceProvider.java	`100.00% <100.00%> (ø)`
... and 36 more

... and 452 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.

HuangFru · 2023-10-17T07:45:12Z

Upgrade googleJavaFormat to 1.15.0 in the Trino module due to:

This version must run with JDK11+, so do not validate in the Trino module and only check it in Trino CI.

zhongqishang · 2023-10-17T08:58:27Z

@HuangFru @shidayang
After the trino module turns on spotless and executes install / package together with other modules, the follow error will be reported.

You are running Spotless on JVM 8, which limits you to google-java-format 1.7.
Upgrade your JVM or try google-java-format alternatives:
- Version 1.7 requires JVM 8+
- Version 1.15.0 requires JVM 11+

I think we need to change the README.md#building script?

HuangFru · 2023-10-17T09:31:27Z

@HuangFru @shidayang After the trino module turns on spotless and executes install / package together with other modules, the follow error will be reported.
You are running Spotless on JVM 8, which limits you to google-java-format 1.7.
Upgrade your JVM or try google-java-format alternatives:
- Version 1.7 requires JVM 8+
- Version 1.15.0 requires JVM 11+
I think we need to change the README.md#building script?

This is a problem. 'mvn validate' must be divided into 'mvn validate -pl '!trino' with jdk8 and 'mvn validate' with jdk17. Maybe applying spotless to the Trino module is not a good idea.

shidayang · 2023-10-17T11:19:06Z

@HuangFru @shidayang After the trino module turns on spotless and executes install / package together with other modules, the follow error will be reported.
You are running Spotless on JVM 8, which limits you to google-java-format 1.7.
Upgrade your JVM or try google-java-format alternatives:
- Version 1.7 requires JVM 8+
- Version 1.15.0 requires JVM 11+
I think we need to change the README.md#building script?
This is a problem. 'mvn validate' must be divided into 'mvn validate -pl '!trino' with jdk8 and 'mvn validate' with jdk17. Maybe applying spotless to the Trino module is not a good idea.

It's really inconvenient that the Spotless plugin cannot use the Toolchain plugin. This PR cannot be merged until this issue is resolved.

HuangFru · 2023-10-19T07:31:08Z

@shidayang @zhongqishang
The Trino module will now be skipped by default. If you need to build the Trino module, you'll need to apply the profile separately. Although Spotless and Toolchains still cannot be used together, other modules and the Trino module have better isolation. The maven command reduces the use of some '-pl' parameters.

# Conflicts: # trino/src/main/java/com/netease/arctic/trino/keyed/KeyedConnectorMetadata.java # trino/src/main/java/com/netease/arctic/trino/unkeyed/IcebergMetadata.java # trino/src/main/java/com/netease/arctic/trino/unkeyed/IcebergSplitManager.java # trino/src/main/java/org/apache/hadoop/hive/metastore/HiveMetaStoreClient.java # trino/src/main/java/org/apache/hadoop/util/VersionInfo.java # trino/src/test/java/com/netease/arctic/trino/iceberg/BaseConnectorTest.java # trino/src/test/java/com/netease/arctic/trino/iceberg/TestBaseArcticConnectorTest.java

zhongqishang · 2023-10-19T15:46:32Z

If we can skip the trino module spotless validate, we can configure <skip>true</skip> by default in trino's pom,
And only add a profile with spotless enabled is enough, such as:

        <profile>
            <id>trino-spotless</id>
            <build>
                <plugins>
                    <plugin>
                        <groupId>com.diffplug.spotless</groupId>
                        <artifactId>spotless-maven-plugin</artifactId>
                        <configuration>
                            <skip>false</skip>
                        </configuration>
                    </plugin>
                </plugins>
            </build>
        </profile>

Then

~~To invoke a build include trino module by toolchains: mvn clean package -DwithTrino -DskipTests -Dspotless.check.skip=true -P toolchain~~
To invoke a build include trino module in Java 17 environment: mvn clean package -DskipTests -P trino-spotless
~~To only build trino and its dependent modules by toolchains: mvn clean package -DwithTrino -DskipTests -Dspotless.check.skip=true -pl 'trino' -am -P toolchain~~
To only build trino and its dependent modules in Java 17 environment: mvn clean package -DskipTests -P trino-spotless -pl 'trino' -am

This can be more concise, WDYT?

shidayang · 2023-10-20T02:21:05Z

If we can skip the trino module spotless validate, we can configure <skip>true</skip> by default in trino's pom, And only add a profile with spotless enabled is enough, such as:
        <profile>
            <id>trino-spotless</id>
            <build>
                <plugins>
                    <plugin>
                        <groupId>com.diffplug.spotless</groupId>
                        <artifactId>spotless-maven-plugin</artifactId>
                        <configuration>
                            <skip>false</skip>
                        </configuration>
                    </plugin>
                </plugins>
            </build>
        </profile>
Then

~~To invoke a build include trino module by toolchains: mvn clean package -DwithTrino -DskipTests -Dspotless.check.skip=true -P toolchain~~

To invoke a build include trino module in Java 17 environment: mvn clean package -DskipTests -P trino-spotless

~~To only build trino and its dependent modules by toolchains: mvn clean package -DwithTrino -DskipTests -Dspotless.check.skip=true -pl 'trino' -am -P toolchain~~

To only build trino and its dependent modules in Java 17 environment: mvn clean package -DskipTests -P trino-spotless -pl 'trino' -am

This can be more concise, WDYT?

Good idea, I can't agree with you more.

HuangFru · 2023-10-20T02:21:43Z

If we can skip the trino module spotless validate, we can configure <skip>true</skip> by default in trino's pom, And only add a profile with spotless enabled is enough, such as:
        <profile>
            <id>trino-spotless</id>
            <build>
                <plugins>
                    <plugin>
                        <groupId>com.diffplug.spotless</groupId>
                        <artifactId>spotless-maven-plugin</artifactId>
                        <configuration>
                            <skip>false</skip>
                        </configuration>
                    </plugin>
                </plugins>
            </build>
        </profile>
Then

~~To invoke a build include trino module by toolchains: mvn clean package -DwithTrino -DskipTests -Dspotless.check.skip=true -P toolchain~~

To invoke a build include trino module in Java 17 environment: mvn clean package -DskipTests -P trino-spotless

~~To only build trino and its dependent modules by toolchains: mvn clean package -DwithTrino -DskipTests -Dspotless.check.skip=true -pl 'trino' -am -P toolchain~~

To only build trino and its dependent modules in Java 17 environment: mvn clean package -DskipTests -P trino-spotless -pl 'trino' -am

This can be more concise, WDYT?

Make sense! I will make changes, thank you for your suggestions.

pom.xml

trino/pom.xml

HuangFru · 2023-10-20T03:01:02Z

Modified it according to @zhongqishang 's suggestions, PTAL @zhongqishang @shidayang @baiyangtx.

zhongqishang

LGTM. Thanks for your work!👍

shidayang · 2023-10-20T08:06:57Z

LGTM

baiyangtx

LGTM

* apply spotless-maven-plugin in trino module. * Revert "apply spotless-maven-plugin in trino module." This reverts commit 7415823. * apply spotless-maven-plugin in trino module. * revert test code. * modify ci not apply checkstyle in trino module in hadoop ci. * add doc to README.md * skip trino module by default * modify CI * remove useless code * modify CI * check style * add a profile to skip trino spotless by default

apply spotless-maven-plugin in trino module.

7415823

github-actions bot added module:mixed-flink Flink moduel for Mixed Format module:mixed-trino trino module for Mixed Format type:build labels Oct 17, 2023

HuangFru closed this Oct 17, 2023

HuangFru added 2 commits October 17, 2023 15:05

Revert "apply spotless-maven-plugin in trino module."

f273c2a

This reverts commit 7415823.

apply spotless-maven-plugin in trino module.

da91b4d

HuangFru reopened this Oct 17, 2023

github-actions bot added module:ams-dashboard Ams dashboard module and removed module:mixed-flink Flink moduel for Mixed Format labels Oct 17, 2023

revert test code.

13b8f95

github-actions bot removed the module:ams-dashboard Ams dashboard module label Oct 17, 2023

modify ci not apply checkstyle in trino module in hadoop ci.

2102f75

github-actions bot added the type:infra label Oct 17, 2023

shidayang approved these changes Oct 17, 2023

View reviewed changes

shidayang closed this Oct 17, 2023

HuangFru reopened this Oct 18, 2023

add doc to README.md

5915063

github-actions bot added the type:docs Improvements or additions to documentation label Oct 18, 2023

HuangFru marked this pull request as draft October 18, 2023 11:19

HuangFru added 4 commits October 19, 2023 15:07

skip trino module by default

1a975e7

modify CI

e0c906f

remove useless code

5351519

modify CI

ad4eabe

HuangFru marked this pull request as ready for review October 19, 2023 07:23

shidayang approved these changes Oct 19, 2023

View reviewed changes

HuangFru added 2 commits October 19, 2023 18:50

check style

38a41f4

baiyangtx requested changes Oct 20, 2023

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

trino/pom.xml Outdated Show resolved Hide resolved

add a profile to skip trino spotless by default

d0a64a3

zhongqishang approved these changes Oct 20, 2023

View reviewed changes

baiyangtx approved these changes Oct 20, 2023

View reviewed changes

baiyangtx merged commit 7de1560 into apache:master Oct 20, 2023
3 of 4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AMORO-2113] Apply spotless-maven-plugin in trino module #2131

[AMORO-2113] Apply spotless-maven-plugin in trino module #2131

HuangFru commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading

HuangFru commented Oct 17, 2023

zhongqishang commented Oct 17, 2023

HuangFru commented Oct 17, 2023

shidayang commented Oct 17, 2023

HuangFru commented Oct 19, 2023

zhongqishang commented Oct 19, 2023

shidayang commented Oct 20, 2023

HuangFru commented Oct 20, 2023

HuangFru commented Oct 20, 2023

zhongqishang left a comment

shidayang commented Oct 20, 2023

baiyangtx left a comment

[AMORO-2113] Apply spotless-maven-plugin in trino module #2131

[AMORO-2113] Apply spotless-maven-plugin in trino module #2131

Conversation

HuangFru commented Oct 17, 2023 • edited Loading

Why are the changes needed?

Brief change log

How was this patch tested?

Documentation

codecov bot commented Oct 17, 2023 • edited Loading

Codecov Report

HuangFru commented Oct 17, 2023

zhongqishang commented Oct 17, 2023

HuangFru commented Oct 17, 2023

shidayang commented Oct 17, 2023

HuangFru commented Oct 19, 2023

zhongqishang commented Oct 19, 2023

shidayang commented Oct 20, 2023

HuangFru commented Oct 20, 2023

HuangFru commented Oct 20, 2023

zhongqishang left a comment

Choose a reason for hiding this comment

shidayang commented Oct 20, 2023

baiyangtx left a comment

Choose a reason for hiding this comment

HuangFru commented Oct 17, 2023 •

edited

Loading

codecov bot commented Oct 17, 2023 •

edited

Loading