-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-6990] [Build] Add Java linting script; fix minor warnings #9867
Closed
Closed
Changes from 33 commits
Commits
Show all changes
38 commits
Select commit
Hold shift + click to select a range
10465b5
[SPARK-6990] Remove whitespace after '>'
dskrvk 38a0007
[SPARK-6990] Add Java linting script
dskrvk 806ae31
[SPARK-6990] Fix some Checkstyle warnings
dskrvk 65db253
[SPARK-6990] Suppress Checkstyle for TimSort
dskrvk 4d7c602
[SPARK-6990] Fix some Checkstyle issues in tests
dskrvk 7ac1bc5
[SPARK-6990] Enable Checkstyle for tests
dskrvk 74dc8df
[SPARK-6990] Enable FallThrough check in Checkstyle
dskrvk 7a88ac2
[SPARK-6990] Fix some Checkstyle warnings
dskrvk 26f6998
[SPARK-6990] Suppress Checkstyle for TimSort
dskrvk 9df236e
[SPARK-6990] Fix some Checkstyle warnings
dskrvk 338f9ae
[SPARK-6990] Suppress Checkstyle for TimSort
dskrvk 2ff1c35
[SPARK-6990] Disable MissingSwitchDefault check
dskrvk 55948e0
[SPARK-6990] Fix qualifier order in SpecificParquetRecordReaderBase
dskrvk 1a57e30
[SPARK-6990] Fix some Checkstyle warnings
dskrvk 59c99c9
[SPARK-6990] Suppress Checkstyle for TimSort
dskrvk d33ed8d
[SPARK-6990] Fix some Checkstyle issues in tests
dskrvk 7d5cbe1
[SPARK-6990] Enable Checkstyle for tests
dskrvk b96a61a
[SPARK-6990] Enable FallThrough check in Checkstyle
dskrvk db17e0e
[SPARK-6990] Disable MissingSwitchDefault check
dskrvk ae30c6a
[SPARK-6990] Fix qualifier order in SpecificParquetRecordReaderBase
dskrvk fd6d0e0
Merge remote-tracking branch 'upstream/master'
dskrvk a9b0393
Merge remote-tracking branch 'upstream/master'
dskrvk 4c27a50
Merge remote-tracking branch 'upstream/master'
dskrvk ce44d2e
Merge remote-tracking branch 'upstream/master'
dskrvk b97a675
Merge branch 'master' into spark-6990
dskrvk 7a49ad7
[SPARK-6990] Fix Checkstyle issues in ML examples
dskrvk 9c6fe92
Merge remote-tracking branch 'upstream/master'
dskrvk 0375281
Merge remote-tracking branch 'upstream/master'
dskrvk 065fe98
Merge remote-tracking branch 'upstream/master'
dskrvk ba90836
Merge remote-tracking branch 'upstream/master'
dskrvk 5b4c7eb
Merge remote-tracking branch 'upstream/master'
dskrvk 1b1a5ab
Merge remote-tracking branch 'upstream/master'
dskrvk c773e90
Merge remote-tracking branch 'upstream/master'
dskrvk c74343b
Merge remote-tracking branch 'upstream/master'
dskrvk b3a192d
Merge remote-tracking branch 'upstream/master'
dskrvk 520e6d2
Merge remote-tracking branch 'upstream/master'
dskrvk f58340c
Merge remote-tracking branch 'upstream/master'
dskrvk b079f29
[SPARK-6990] Remove second run of mvn checkstyle
dskrvk File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
<!-- | ||
~ 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"> | ||
|
||
<!-- | ||
|
||
This file contains suppression rules for Checkstyle checks. | ||
Ideally only files that cannot be modified (e.g. third-party code) | ||
should be added here. All other violations should be fixed. | ||
|
||
--> | ||
|
||
<suppressions> | ||
<suppress checks=".*" | ||
files="core/src/main/java/org/apache/spark/util/collection/TimSort.java"/> | ||
</suppressions> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,164 @@ | ||
<!-- | ||
~ 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 module PUBLIC | ||
"-//Puppy Crawl//DTD Check Configuration 1.3//EN" | ||
"http://www.puppycrawl.com/dtds/configuration_1_3.dtd"> | ||
|
||
<!-- | ||
|
||
Checkstyle configuration based on the Google coding conventions from: | ||
|
||
- Google Java Style | ||
https://google-styleguide.googlecode.com/svn-history/r130/trunk/javaguide.html | ||
|
||
with Spark-specific changes from: | ||
|
||
https://cwiki.apache.org/confluence/display/SPARK/Spark+Code+Style+Guide | ||
|
||
Checkstyle is very configurable. Be sure to read the documentation at | ||
http://checkstyle.sf.net (or in your downloaded distribution). | ||
|
||
Most Checks are configurable, be sure to consult the documentation. | ||
|
||
To completely disable a check, just comment it out or delete it from the file. | ||
|
||
Authors: Max Vetrenko, Ruslan Diachenko, Roman Ivanov. | ||
|
||
--> | ||
|
||
<module name = "Checker"> | ||
<property name="charset" value="UTF-8"/> | ||
|
||
<property name="severity" value="error"/> | ||
|
||
<property name="fileExtensions" value="java, properties, xml"/> | ||
|
||
<module name="SuppressionFilter"> | ||
<property name="file" value="checkstyle-suppressions.xml"/> | ||
</module> | ||
|
||
<!-- Checks for whitespace --> | ||
<!-- See http://checkstyle.sf.net/config_whitespace.html --> | ||
<module name="FileTabCharacter"> | ||
<property name="eachLine" value="true"/> | ||
</module> | ||
|
||
<module name="TreeWalker"> | ||
<module name="OuterTypeFilename"/> | ||
<module name="IllegalTokenText"> | ||
<property name="tokens" value="STRING_LITERAL, CHAR_LITERAL"/> | ||
<property name="format" value="\\u00(08|09|0(a|A)|0(c|C)|0(d|D)|22|27|5(C|c))|\\(0(10|11|12|14|15|42|47)|134)"/> | ||
<property name="message" value="Avoid using corresponding octal or Unicode escape."/> | ||
</module> | ||
<module name="AvoidEscapedUnicodeCharacters"> | ||
<property name="allowEscapesForControlCharacters" value="true"/> | ||
<property name="allowByTailComment" value="true"/> | ||
<property name="allowNonPrintableEscapes" value="true"/> | ||
</module> | ||
<!-- TODO: 11/09/15 disabled - the lengths are currently > 100 in many places --> | ||
<!-- | ||
<module name="LineLength"> | ||
<property name="max" value="100"/> | ||
<property name="ignorePattern" value="^package.*|^import.*|a href|href|http://|https://|ftp://"/> | ||
</module> | ||
--> | ||
<module name="NoLineWrap"/> | ||
<module name="EmptyBlock"> | ||
<property name="option" value="TEXT"/> | ||
<property name="tokens" value="LITERAL_TRY, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE, LITERAL_SWITCH"/> | ||
</module> | ||
<module name="NeedBraces"> | ||
<property name="allowSingleLineStatement" value="true"/> | ||
</module> | ||
<module name="OneStatementPerLine"/> | ||
<module name="ArrayTypeStyle"/> | ||
<module name="FallThrough"/> | ||
<module name="UpperEll"/> | ||
<module name="ModifierOrder"/> | ||
<module name="SeparatorWrap"> | ||
<property name="tokens" value="DOT"/> | ||
<property name="option" value="nl"/> | ||
</module> | ||
<module name="SeparatorWrap"> | ||
<property name="tokens" value="COMMA"/> | ||
<property name="option" value="EOL"/> | ||
</module> | ||
<module name="PackageName"> | ||
<property name="format" value="^[a-z]+(\.[a-z][a-z0-9]*)*$"/> | ||
<message key="name.invalidPattern" | ||
value="Package name ''{0}'' must match pattern ''{1}''."/> | ||
</module> | ||
<module name="ClassTypeParameterName"> | ||
<property name="format" value="([A-Z][a-zA-Z0-9]*$)"/> | ||
<message key="name.invalidPattern" | ||
value="Class type name ''{0}'' must match pattern ''{1}''."/> | ||
</module> | ||
<module name="MethodTypeParameterName"> | ||
<property name="format" value="([A-Z][a-zA-Z0-9]*)"/> | ||
<message key="name.invalidPattern" | ||
value="Method type name ''{0}'' must match pattern ''{1}''."/> | ||
</module> | ||
<module name="NoFinalizer"/> | ||
<module name="GenericWhitespace"> | ||
<message key="ws.followed" | ||
value="GenericWhitespace ''{0}'' is followed by whitespace."/> | ||
<message key="ws.preceded" | ||
value="GenericWhitespace ''{0}'' is preceded with whitespace."/> | ||
<message key="ws.illegalFollow" | ||
value="GenericWhitespace ''{0}'' should followed by whitespace."/> | ||
<message key="ws.notPreceded" | ||
value="GenericWhitespace ''{0}'' is not preceded with whitespace."/> | ||
</module> | ||
<!-- TODO: 11/09/15 disabled - indentation is currently inconsistent --> | ||
<!-- | ||
<module name="Indentation"> | ||
<property name="basicOffset" value="4"/> | ||
<property name="braceAdjustment" value="0"/> | ||
<property name="caseIndent" value="4"/> | ||
<property name="throwsIndent" value="4"/> | ||
<property name="lineWrappingIndentation" value="4"/> | ||
<property name="arrayInitIndent" value="4"/> | ||
</module> | ||
--> | ||
<!-- TODO: 11/09/15 disabled - order is currently wrong in many places --> | ||
<!-- | ||
<module name="ImportOrder"> | ||
<property name="separated" value="true"/> | ||
<property name="ordered" value="true"/> | ||
<property name="groups" value="/^javax?\./,scala,*,org.apache.spark"/> | ||
</module> | ||
--> | ||
<module name="MethodParamPad"/> | ||
<module name="AnnotationLocation"> | ||
<property name="tokens" value="CLASS_DEF, INTERFACE_DEF, ENUM_DEF, METHOD_DEF, CTOR_DEF"/> | ||
</module> | ||
<module name="AnnotationLocation"> | ||
<property name="tokens" value="VARIABLE_DEF"/> | ||
<property name="allowSamelineMultipleAnnotations" value="true"/> | ||
</module> | ||
<module name="MethodName"> | ||
<property name="format" value="^[a-z][a-z0-9][a-zA-Z0-9_]*$"/> | ||
<message key="name.invalidPattern" | ||
value="Method name ''{0}'' must match pattern ''{1}''."/> | ||
</module> | ||
<module name="EmptyCatchBlock"> | ||
<property name="exceptionVariableName" value="expected"/> | ||
</module> | ||
<module name="CommentsIndentation"/> | ||
</module> | ||
</module> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
#!/usr/bin/env bash | ||
|
||
# | ||
# 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. | ||
# | ||
|
||
SCRIPT_DIR="$( cd "$( dirname "$0" )" && pwd )" | ||
SPARK_ROOT_DIR="$(dirname $SCRIPT_DIR)" | ||
|
||
$SCRIPT_DIR/../build/mvn -Pkinesis-asl -Phive -Phive-thriftserver checkstyle:check > checkstyle.txt | ||
$SCRIPT_DIR/../build/mvn -Pkinesis-asl -Pyarn -Phadoop-2.2 checkstyle:check >> checkstyle.txt | ||
|
||
ERRORS=$(grep ERROR checkstyle.txt) | ||
rm checkstyle.txt | ||
|
||
if test ! -z "$ERRORS"; then | ||
echo -e "Checkstyle checks failed at following occurrences:\n$ERRORS" | ||
exit 1 | ||
else | ||
echo -e "Checkstyle checks passed." | ||
fi |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick question: why do you need to run twice with different profiles? AFAIK the set of source files should be the same under all of the Hadoop profiles, so I don't think we need to set
-Phadoop-2.2
here.Why can't we just use one Maven run with the profiles
-Pkinesis-asl -Phive -Phive-thriftserver -Pyarn
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't actually observe any differences between the two profiles in terms of Checkstyle warnings, but decided to add the second run just to be thorough.
My reasoning was that since some profiles omit some of the modules, we need to exercise all of the possible ones, even though at the moment the set of Java sources may be the same. In any case, this only adds a few seconds to the build - negligible compared to the overall
run-tests
time.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to have a single run with "-Pkinesis-asl -Pyarn -Phive -Phive-thriftserver" - I even think "-Phive" is unnecessary, I think it only affects packaging right now.
"-Phadoop2.2" is unnecessary, that's the default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; changed.