[SPARK-48214][INFRA] Ban import org.slf4j.Logger & org.slf4j.LoggerFactory

[panbingkun]

What changes were proposed in this pull request?

The pr aims to ban import org.slf4j.Logger & org.slf4j.LoggerFactory.

Why are the changes needed?

After the migration of structured logs on the java side is completed, we need to ban import org.slf4j.Logger & org.slf4j.LoggerFactory in the code to avoid the log format that is not written as required in the future new java code.

Does this PR introduce any user-facing change?

Yes, only for spark developers.

How was this patch tested?

• Manually test.
• Pass GA.

Was this patch authored or co-authored using generative AI tooling?

No.

[dongjoon-hyun]

It seems to reveal that we have too many exceptions for this new rule though.

[panbingkun]

• According to @gengliangwang's suggestion, we did not migrate the test code in the structured log, so we need to exclude them, eg:

• Other exclusion is: common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java

A.the module `common/kvstore`, because it does not rely on 'utils' when compiling, if we want to `migrate` it, we need to add a `dependency` on 'utils' in `pom.xml` https://github.com/apache/spark/blob/master/common/kvstore/pom.xml#L38-L66

B.And only one place in this module use 'slf4j', as shown below:

spark/common/kvstore/src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java

Line 324 in e1fb1d7

LOG.warn("Failed to close iterator", e);

And we found that this error level log does not use variables
So at present, it seems that migration is not necessary. Of course, migration is also possible.

• After complete the last structured log migration pr https://github.com/apache/spark/pull/46493/files on the java side, this rule should be applied to the spark code base (I have tested it on local env)

[dongjoon-hyun]

cc @gengliangwang

[gengliangwang]

I am +1 for the idea.
However, I wonder if there will be suggestions about why the two imports are not allowed and how to fix the style error.
If that's not feasible with IllegalImport, shall we use RegexpSinglelineJava and show proper suggestions instead?

[panbingkun]

I am +1 for the idea. However, I wonder if there will be suggestions about why the two imports are not allowed and how to fix the style error. If that's not feasible with IllegalImport, shall we use RegexpSinglelineJava and show proper suggestions instead?

Well, it makes sense,
the illegalimport does not provide a friendly prompt information interface,
let me to update it later.

[LuciferYang]

Or how about having these modules depend on the common/utils module? common/utils doesn't seem to be a heavyweight module, in this way, the existing cases can be fixed.

[panbingkun]

sh dev/lint-java

Using `mvn` from path: /Users/panbingkun/Developer/infra/maven/maven/bin/mvn
-e Checkstyle checks failed at following occurrences:
[ERROR] src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java:[33] (regexp) RegexpSinglelineJava: Please use org.apache.spark.internal.(Logger|LoggerFactory) instead.
[ERROR] src/main/java/org/apache/spark/util/kvstore/LevelDBIterator.java:[34] (regexp) RegexpSinglelineJava: Please use org.apache.spark.internal.(Logger|LoggerFactory) instead

[panbingkun]

Or how about having these modules depend on the common/utils module? common/utils doesn't seem to be a heavyweight module, in this way, the existing cases can be fixed.

Done.

[panbingkun]

Ready for it, @gengliangwang @dongjoon-hyun @LuciferYang

[gengliangwang]

@panbingkun let's hold on this until we have a conclusion in #46600

[panbingkun]

@panbingkun let's hold on this until we have a conclusion in #46600

@gengliangwang
After #46600, the pr has updated.

[gengliangwang]

yeah this looks better

[gengliangwang]

Thanks, merging to master