[CELEBORN-1070] Add error-prone to pom.xml.

[mridulm]

What changes were proposed in this pull request?

Add error prone to the build.
Error Prone is a static analysis tool that cam catch common bugs and mistakes during compilation.

Why are the changes needed?

Catch potential issues during build

Does this PR introduce any user-facing change?

No

How was this patch tested?

Local build

[mridulm]

This is resubmission of #2016.
I was testing it on JDK 11 and later locally - and so did not observe the failures that build system surfaced.
Have added workarounds to fix this, as per error prone documentation.

+CC @waitinfuture

[SteNicholas]

@mridulm, could you also test it on JDK17? At present Celeborn supports JDK17.

[mridulm]

So looks like there are more failures to be fixed in flink related profiles.
Will convert this back to draft and address those first.

[waitinfuture]

@mridulm, could you also test it on JDK17? At present Celeborn supports JDK17.

Seems GA with JDK 17 fails.

[mridulm]

Yes, looks like JDK 17 has some specific instructions as well - did not realize Celeborn supports jdk 17 :) will add those as well, after I fix the flink related failures.

[pan3793]

if applicable, would you mind adding such compiling improvement to sbt building system as well?

background: in the previous discussion, we'd like to migrate the building system from maven to sbt because:

• sbt is much faster than maven
• Spark is going to use sbt only in 4.0.0
• sbt shell is convenient for testing during development
• maven scalatest plugin does not support always fork mode https://github.com/apache/incubator-celeborn/blob/2758df713ead7fd78fdee9f8a2af2ad2ce377fa9/pom.xml#L616-L620

the main process is driven by @cfmcgrady, and the first step is to make sbt align with maven(almost reached), and then drop maven ...

[SteNicholas]

Yes, looks like JDK 17 has some specific instructions as well - did not realize Celeborn supports jdk 17 :) will add those as well, after I fix the flink related failures.

@mridulm, the build section has documented the situation of JDK17 support. You could refer this section.

[codecov]

Codecov Report

Merging #2025 (4c2ea29) into main (6dc30d3) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2025      +/-   ##
==========================================
+ Coverage   46.90%   46.92%   +0.03%     
==========================================
  Files         165      165              
  Lines       10531    10535       +4     
  Branches      959      958       -1     
==========================================
+ Hits         4938     4943       +5     
- Misses       5271     5272       +1     
+ Partials      322      320       -2     

see 6 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mridulm]

@pan3793
You mentioned:

maven scalatest plugin does not support always fork mode

I am currently running with fork mode = true for all the tests with maven and it is running successfully - can you give more details on this please ? There are so many combinations, I might have missed some !

the main process is driven by @cfmcgrady, and the first step is to make sbt align with maven(almost reached), and then drop maven ...

I did not find sbt support details, can you point me to it ?
I would prefer to add the support as a follow up if it is not yet merged.

[mridulm]

The flink failures will be addressed by #2029

[mridulm]

+CC @waitinfuture , @SteNicholas All the tests pass now

[waitinfuture]

LGTM, thanks!

[waitinfuture]

@pan3793 You mentioned:

maven scalatest plugin does not support always fork mode

I am currently running with fork mode = true for all the tests with maven and it is running successfully - can you give more details on this please ? There are so many combinations, I might have missed some !

the main process is driven by @cfmcgrady, and the first step is to make sbt align with maven(almost reached), and then drop maven ...

I did not find sbt support details, can you point me to it ? I would prefer to add the support as a follow up if it is not yet merged.

Here is sbt doc https://celeborn.apache.org/docs/latest/developers/sbt/

[pan3793]

I am currently running with fork mode = true for all the tests with maven and it is running successfully

Yes, we fixed several cases to make it pass now.

Basically, when setting forkMode=once, all test cases run on the same forked JVM process, which does not much help for projects that compose a lot of singletons(if we initialize one singleton in the previous test case, the following test cases may be affected, so we must add some reset logic for testing purposes)

The ideal solution is forkMode=always, running each test case in its own forked JVM process, unfortunately, Maven JUnit plugin supports it but scalatest does not.

https://github.com/scalatest/scalatest-maven-plugin/blob/383f396162b7654930758b76a0696d3aa2ce5686/src/main/java/org/scalatest/tools/maven/AbstractScalaTestMojo.java#L162-L168

I did not find sbt support details ...

Currently, we only provide developer docs with sbt

https://celeborn.apache.org/docs/latest/developers/sbt/

[SteNicholas]

Suggested change

	<version>3.11.0</version>
	<version>${maven.plugin.compiler.version}</version>

[SteNicholas]

LGTM. Thanks for @mridulm updates.

[pan3793]

Thanks, merged to main for 0.4

[pan3793]

@mridulm seems you didn't set up the git email address properly, mridulatgmail.com, would you mind replacing the at with @ in your local git email setup? So that the GitHub could correctly link the commit to your GitHub account.

PS: I fixed the email when merging so that the GitHub link works, I saw the unrecognized commits that you merged in the Spark repo.

[mridulm]

That is done on purpose to minimize spam

[pan3793]

alright...