[FLINK-36542] Enable upToDateChecking to speed up the spotless

[ruanwenjun]

What is the purpose of the change

Speed up the spotless. JIRA: https://issues.apache.org/jira/browse/FLINK-36542

Brief change log

• Enable upToDateChecking in spotless-maven-plugin

Verifying this change

Use mvn spotless:apply or mvn spotless:check to verify.

Does this pull request potentially affect one of the following parts:

• Dependencies (does it add or upgrade a dependency): no
• The public API, i.e., is any changed class annotated with @Public(Evolving): no
• The serializers: no
• The runtime per-record code paths (performance sensitive): no
• Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
• The S3 file system connector: no

Documentation

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

[flinkbot]

CI report:

• cbbca10570b59f9d8d59da754fb5cbe567f90b63 Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

• @flinkbot run azure re-run the last Azure build

[vahmed-hamdy]

Nice quick optimisation, thanks!

[ruanwenjun]

Nice quick optimisation, thanks!

I am very happy to be able to make something that will help flink!

[ferenc-csaky]

I think this would be pretty useful, just wondering about if we enable it by default could it break something in any CI? My understanding is that by default the index file is placed into the project root, and I think that would break the RAT license checker. But even if not, or if we add an exception for the index in the RAT config doing this extra step in CI runs has no much value, as that always runs start a clean slate, cloning the code, etc.

All in all, I think disabling this by default would be a safer choice, we can introduce a property for it to make it easy to enable indexing via a dynamic param in any local run. WDYT?

[ruanwenjun]

@ferenc-csaky Thanks for your review, as far as I know, the index file will be generated under root/target, this will not be put into binary package, so will not break the RAT license check, we don't need any extra change in CI.

BTW, disable it by default and enable this by using maven args is acceptable to me.

mvn spotless:apply -Dspotless.usingIndex=true

It's not a burden to me.

[ferenc-csaky]

@ferenc-csaky Thanks for your review, as far as I know, the index file will be generated under root/target, this will not be put into binary package, so will not break the RAT license check, we don't need any extra change in CI.

BTW, disable it by default and enable this by using maven args is acceptable to me.

mvn spotless:apply -Dspotless.usingIndex=true

It's not a burden to me.

Sounds good to me! Thank you!

[ferenc-csaky]

@flinkbot run azure

[jnh5y]

I think it'd be better to enable this by default (if there are concerns about CI, it could be disabled there), and also to upgrade the Spotless plugin version.

The default is for this feature to be enabled in 2.35.0 and higher: https://github.com/diffplug/spotless/tree/main/plugin-maven#incremental-up-to-date-checking-and-formatting

[ruanwenjun]

I think it'd be better to enable this by default (if there are concerns about CI, it could be disabled there), and also to upgrade the Spotless plugin version.

The default is for this feature to be enabled in 2.35.0 and higher: https://github.com/diffplug/spotless/tree/main/plugin-maven#incremental-up-to-date-checking-and-formatting

Thanks for remind this, I agree with upgrade spotless plugin.

Here are some things I want to confirm

1. If we upgrade the spotless to 2.35.0, then where do we disable it, in CI or disable it by default? Is there any way we can know the effect of opening on CI? I got the result from flinkbot is SUCCESS.
2. Do we upgrade in this PR or submit a separate PR to upgrade？

[jnh5y]

Here are some things I want to confirm

1. If we upgrade the spotless to 2.35.0, then where do we disable it, in CI or disable it by default? Is there any way we can know the effect of opening on CI? I got the result from flinkbot is SUCCESS.

In a single run of CI, spotless is not going to fail the build. It may slightly slow it down since there'd be extra IO to write whatever output it writes.

My opinion is that we do not need to care and do not need to override anything. I have two reasons:

A. The Flink build is already complex enough; fewer options is better. B. The maintainers for the Spotless plugin have made this setting the default. If it were causing issues, they would not have done so.

2. Do we upgrade in this PR or submit a separate PR to upgrade？

Given my position above, I would suggest we update the version instead of exposing the configuration that this PR does. If @ferenc-csaky / the community is ok with that, then it just comes down to a question of updating this PR versus a new one. (And on that point, I do not have an opinion.)

[ferenc-csaky]

@jnh5y @ruanwenjun Thanks for the added thoughts! I am okay with updating this PR with simply bumping the spotless version to 2.35.0 instead of what is happening currently in a different commit and force push the PR branch. Putting the index file under target/ is not harmful at all, so my original point does not matter anyway.

@ruanwenjun Would you mind adapting the Jira ticket ticket summary/desc and the PR title accordingly as well?

[ruanwenjun]

@ferenc-csaky Done, updated the PR title and jira.

[ferenc-csaky]

@ruanwenjun can you pls. rebase on top of the latest master and force-push again? That wikipedia example got broken a couple days ago, but since then it was removed, so it does not block the CI anymore. I know it is not related to this change at all, but in general it is a good practice to always have a green CI. Sorry about the back and forth. :)

[ruanwenjun]

Done.