You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by zentol <gi...@git.apache.org> on 2017/06/12 15:59:23 UTC
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
GitHub user zentol opened a pull request:
https://github.com/apache/flink/pull/4112
[FLINK-6901] Flip checkstyle configuration files
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.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/zentol/flink 6901
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/flink/pull/4112.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #4112
----
commit 007ef6d66835626bebec2499d9b23ba1d829264f
Author: zentol <ch...@apache.org>
Date: 2017-06-12T15:40:21Z
[FLINK-6902] Activate strict checkstyle for flink-streaming-scala
commit 0c89dd3b3df44ed8fda93bf5eb9dbfb2aa7dc28c
Author: zentol <ch...@apache.org>
Date: 2017-06-12T15:44:51Z
[FLINK-6901] Remove plugins using strict-checkstyle.xml
commit 8887e6cb407f92388937cec16172c6840637c3be
Author: zentol <ch...@apache.org>
Date: 2017-06-12T15:45:45Z
[FLINK-6901] Rework exclusions
commit edf048734edb1d0d36a92a082d228207bc5ddbf3
Author: zentol <ch...@apache.org>
Date: 2017-06-12T15:46:16Z
[FLINK-6901] Replace lenient checkstyle
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4112
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.
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4112
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?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by zentol <gi...@git.apache.org>.
Github user zentol closed the pull request at:
https://github.com/apache/flink/pull/4112
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4112#discussion_r121480878
--- Diff: tools/maven/suppressions-runtime.xml ---
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions 1.1//EN"
+ "http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">
+
+<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/(.*)"
--- End diff --
yes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by NicoK <gi...@git.apache.org>.
Github user NicoK commented on a diff in the pull request:
https://github.com/apache/flink/pull/4112#discussion_r126451783
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/NettyBufferPool.java ---
@@ -61,7 +60,9 @@ public NettyBufferPool(int numberOfArenas) {
checkArgument(numberOfArenas >= 1, "Number of arenas");
this.numberOfArenas = numberOfArenas;
- if (!PlatformDependent.hasUnsafe()) {
+ try {
+ Class.forName("sun.misc.Unsafe");
+ } catch (ClassNotFoundException e) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4112#discussion_r121495764
--- Diff: tools/maven/suppressions-runtime.xml ---
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions 1.1//EN"
+ "http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">
+
+<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/(.*)"
--- End diff --
I've split the suppressions by package.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/4112#discussion_r121465225
--- Diff: 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++) {
--- End diff --
What is the reason for these two changes in `flink-core`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on the issue:
https://github.com/apache/flink/pull/4112
yes, i intended to include FLINK-6902, as described in the second paragraph of the PR description.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by zentol <gi...@git.apache.org>.
Github user zentol commented on a diff in the pull request:
https://github.com/apache/flink/pull/4112#discussion_r121480837
--- Diff: 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++) {
--- End diff --
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by uce <gi...@git.apache.org>.
Github user uce commented on a diff in the pull request:
https://github.com/apache/flink/pull/4112#discussion_r126464199
--- Diff: flink-runtime/src/main/java/org/apache/flink/runtime/io/network/netty/NettyBufferPool.java ---
@@ -61,7 +60,9 @@ public NettyBufferPool(int numberOfArenas) {
checkArgument(numberOfArenas >= 1, "Number of arenas");
this.numberOfArenas = numberOfArenas;
- if (!PlatformDependent.hasUnsafe()) {
+ try {
+ Class.forName("sun.misc.Unsafe");
+ } catch (ClassNotFoundException e) {
--- End diff --
Is ok with me to remove this check
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/4112
@zentol did you mean to include `[FLINK-6902] Activate strict checkstyle for flink-streaming-scala` as the first commit?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink pull request #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on a diff in the pull request:
https://github.com/apache/flink/pull/4112#discussion_r121463726
--- Diff: tools/maven/suppressions-runtime.xml ---
@@ -0,0 +1,29 @@
+<?xml version="1.0"?>
+<!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+-->
+
+<!DOCTYPE suppressions PUBLIC
+ "-//Puppy Crawl//DTD Suppressions 1.1//EN"
+ "http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">
+
+<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/(.*)"
--- End diff --
Should we duplicate the listing one package per `suppress` tag to prevent merge conflicts when removing excluded packages?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/4112
LGTM on a second look. Don't have any suggestion on NettyBufferPool. @uce?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] flink issue #4112: [FLINK-6901] Flip checkstyle configuration files
Posted by greghogan <gi...@git.apache.org>.
Github user greghogan commented on the issue:
https://github.com/apache/flink/pull/4112
+1 and my apologies for overlooking your comprehensive description.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---