[FLINK-6901] Flip checkstyle configuration files #4112

zentol · 2017-06-12T15:59:20Z

This PR makes the strict-checkstyle.xml the default checkstyle configuration.

As a first step i made flink-streaming-scala checkstyle compliant (FLINK-6902), because it required few changes and i didn't want to add an exclusion only to remove it again tomorrow.

Making the strict checkstyle the default required a few more changes than i expected. As it stands it is not possible to execute the checkstyle plugin twice in a single build with different configurations. This also implies that currently many parts of flink-runtime aren't even covered by the lenient checkstyle.

So we need a way to both check the lenient for all files, and the strict for a subset in a single run. Given that the strict checkstyle is an extension of the lenient one i instead opted for creating suppression files for each of the non-covered modules, disabling all the new checks added by the strict checkstyle. This didn't work out completely as it still required resolving 2 violations in flink-core, but that's ok i guess.

The suppression files are pretty ugly as they consist of a giant regex checking for every module in a single line, and the same for packages in flink-runtime, but i couldn't find a better solution. Spreading the regex over multiple-lines frustratingly enough doesn't work.

One could resolve this somewhat for flink-runtime by adding a separate suppression for every package, but you would still retain the endless rules regex.

Note that this change implies that every new module is now covered by the strict checkstyle.

greghogan · 2017-06-12T16:34:30Z

@zentol did you mean to include [FLINK-6902] Activate strict checkstyle for flink-streaming-scala as the first commit?

greghogan · 2017-06-12T16:40:06Z

tools/maven/suppressions-runtime.xml

+
+<suppressions>
+	<suppress
+		files="	(.*)runtime/akka/(.*)|(.*)runtime/blob/(.*)|(.*)runtime/broadcast/(.*)|(.*)runtime/checkpoint/(.*)|(.*)runtime/client/(.*)|(.*)runtime/clusterframework/(.*)|(.*)runtime/concurrent/(.*)|(.*)runtime/deployment/(.*)|(.*)runtime/execution/(.*)|(.*)runtime/executiongraph/(.*)|(.*)runtime/fs/(.*)|(.*)runtime/heartbeat/(.*)|(.*)runtime/highavailability/(.*)|(.*)runtime/instance/(.*)|(.*)runtime/io/(.*)|(.*)runtime/iterative/(.*)|(.*)runtime/jobgraph/(.*)|(.*)runtime/jobmanager/(.*)|(.*)runtime/jobmaster/(.*)|(.*)runtime/leaderelection/(.*)|(.*)runtime/memory/(.*)|(.*)runtime/messages/(.*)|(.*)runtime/minicluster/(.*)|(.*)runtime/net/(.*)|(.*)runtime/operators/(.*)|(.*)runtime/plugable/(.*)|(.*)runtime/query/(.*)|(.*)runtime/registration/(.*)|(.*)runtime/resourcemanager/(.*)|(.*)runtime/rpc/(.*)|(.*)runtime/security/(.*)|(.*)runtime/state/(.*)|(.*)runtime/taskexecutor/(.*)|(.*)runtime/taskmanager/(.*)|(.*)runtime/testutils/(.*)|(.*)runtime/util/(.*)|(.*)runtime/zookeeper/(.*)"


Should we duplicate the listing one package per suppress tag to prevent merge conflicts when removing excluded packages?

I've split the suppressions by package.

greghogan · 2017-06-12T16:46:16Z

flink-core/src/main/java/org/apache/flink/util/StringValueUtils.java

@@ -114,15 +114,17 @@ public boolean next(StringValue target) {
 			int pos = this.pos;

 			// skip the delimiter
-			for (; pos < limit && Character.isWhitespace(data[pos]); pos++);
+			for (; pos < limit && Character.isWhitespace(data[pos]); pos++) {


What is the reason for these two changes in flink-core?

The NeedBraces module was extended with the strict-checkstyle to also apply to do, while and for.

Since i can't selectively disable parts of the module I had to make the changes instead.

zentol · 2017-06-12T17:47:21Z

yes, i intended to include FLINK-6902, as described in the second paragraph of the PR description.

greghogan · 2017-06-12T19:49:54Z

+1 and my apologies for overlooking your comprehensive description.

zentol · 2017-06-13T08:28:31Z

Looks like the same problem we have in flink-core also exists in flink-runtime, some braces are missing...

[INFO] There are 10 errors reported by Checkstyle 6.19 with /tools/maven/checkstyle.xml ruleset.
[ERROR] src/main/java/org/apache/flink/runtime/broadcast/BroadcastVariableMaterialization.java:[134] (blocks) NeedBraces: 'while' construct must use '{}'s.
[ERROR] src/main/java/org/apache/flink/runtime/io/network/netty/NettyBufferPool.java:[25,1] (imports) IllegalImport: Import from illegal package - io.netty.util.internal.PlatformDependent.
[ERROR] src/main/java/org/apache/flink/runtime/iterative/task/AbstractIterativeTask.java:[247] (blocks) NeedBraces: 'while' construct must use '{}'s.
[ERROR] src/main/java/org/apache/flink/runtime/operators/AbstractCachedBuildSideJoinDriver.java:[176] (blocks) NeedBraces: 'while' construct must use '{}'s.
[ERROR] src/main/java/org/apache/flink/runtime/operators/AbstractOuterJoinDriver.java:[160] (blocks) NeedBraces: 'while' construct must use '{}'s.
[ERROR] src/main/java/org/apache/flink/runtime/operators/JoinDriver.java:[222] (blocks) NeedBraces: 'while' construct must use '{}'s.
[ERROR] src/main/java/org/apache/flink/runtime/operators/sort/AbstractMergeInnerJoinIterator.java:[68] (blocks) NeedBraces: 'while' construct must use '{}'s.
[ERROR] src/main/java/org/apache/flink/runtime/operators/sort/AbstractMergeInnerJoinIterator.java:[69] (blocks) NeedBraces: 'while' construct must use '{}'s.
[ERROR] src/main/java/org/apache/flink/runtime/operators/sort/AbstractMergeOuterJoinIterator.java:[92] (blocks) NeedBraces: 'while' construct must use '{}'s.
[ERROR] src/main/java/org/apache/flink/runtime/operators/sort/AbstractMergeOuterJoinIterator.java:[103] (blocks) NeedBraces: 'while' construct must use '{}'s.

zentol · 2017-06-13T08:52:48Z

I've adjusted the suppressions file to properly work on windows (slashes...), and resolves the few issues in flink-runtime.

I'm a bit unsure about the change in the NettyBufferPool, @uce does that look ok to you?

greghogan · 2017-07-07T19:16:21Z

LGTM on a second look. Don't have any suggestion on NettyBufferPool. @uce?

NicoK · 2017-07-10T15:15:44Z

flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/NettyBufferPool.java

-		if (!PlatformDependent.hasUnsafe()) {
+		try {
+			Class.forName("sun.misc.Unsafe");
+		} catch (ClassNotFoundException e) {


After checking with @StefanRRichter, it seems this check is unnecessary anyway since org.apache.flink.core.memory.MemoryUtils#UNSAFE is getting the class if needed and if not existing, Flink should not start anyway.

It's ok with me to remove this check

greghogan reviewed Jun 12, 2017

View reviewed changes

zentol mentioned this pull request Jun 13, 2017

[FLINK-6865] Update checkstyle documentation #4086

Closed

zentol force-pushed the 6901 branch from 8fb401c to 393e087 Compare July 1, 2017 12:27

zentol mentioned this pull request Jul 1, 2017

[FLINK-6695] Activate strict checkstyle for flink-contrib #4231

Closed

zentol force-pushed the 6901 branch 2 times, most recently from 536d7ac to a09d8e3 Compare July 1, 2017 14:53

NicoK reviewed Jul 10, 2017

View reviewed changes

zentol mentioned this pull request Jul 12, 2017

[FLINK-6835] Update code style documentation apache/flink-web#63

Merged

zentol force-pushed the 6901 branch 5 times, most recently from 671134f to 76027e3 Compare July 13, 2017 05:52

zentol added 2 commits July 13, 2017 09:12

[FLINK-6902] Activate strict checkstyle for flink-streaming-scala

b2faa3e

[FLINK-6901] Make strict-checkstyle the default

4e4a0a7

zentol force-pushed the 6901 branch from 76027e3 to 4e4a0a7 Compare July 13, 2017 07:12

zentol closed this Jul 13, 2017

zentol deleted the 6901 branch July 13, 2017 09:55

rmetzger added the component=BuildSystem label Mar 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-6901] Flip checkstyle configuration files #4112

[FLINK-6901] Flip checkstyle configuration files #4112

zentol commented Jun 12, 2017

greghogan commented Jun 12, 2017

greghogan Jun 12, 2017

zentol Jun 12, 2017

zentol Jun 12, 2017

greghogan Jun 12, 2017

zentol Jun 12, 2017

zentol commented Jun 12, 2017

greghogan commented Jun 12, 2017

zentol commented Jun 13, 2017

zentol commented Jun 13, 2017

greghogan commented Jul 7, 2017

NicoK Jul 10, 2017

uce Jul 10, 2017 •

edited

[FLINK-6901] Flip checkstyle configuration files #4112

[FLINK-6901] Flip checkstyle configuration files #4112

Conversation

zentol commented Jun 12, 2017

greghogan commented Jun 12, 2017

greghogan Jun 12, 2017

Choose a reason for hiding this comment

zentol Jun 12, 2017

Choose a reason for hiding this comment

zentol Jun 12, 2017

Choose a reason for hiding this comment

greghogan Jun 12, 2017

Choose a reason for hiding this comment

zentol Jun 12, 2017

Choose a reason for hiding this comment

zentol commented Jun 12, 2017

greghogan commented Jun 12, 2017

zentol commented Jun 13, 2017

zentol commented Jun 13, 2017

greghogan commented Jul 7, 2017

NicoK Jul 10, 2017

Choose a reason for hiding this comment

uce Jul 10, 2017 • edited

Choose a reason for hiding this comment

uce Jul 10, 2017 •

edited