Remove redundant type parameters and enforce some other style and inspection rules by asdf2014 · Pull Request #5980 · apache/druid

asdf2014 · 2018-07-09T08:48:56Z

Various changes:

Fix calling to toArray() with pre-sized array argument for better performance;
Use lambda to complete the initialization of Module / Runnable / Accumulator;
Remove unuesed import;
Improve if-return clause;
Use Set#addAll instead of for loop;
Use Collections.<...>singletonList instead of Arrays.<...>asList for single element situation;
Use file.toURI().toURL() instead of file.toURL();
Remove unnecessary unboxing;
Put the string object in front of the equals method;
Use Collections.<StorageLocationConfig>emptyList() instead of Arrays.<StorageLocationConfig>asList();
Add a few coding inspection rules, includes ArraysAsListWithZeroOrOneArgument, RedundantTypeArguments and ToArrayCallWithZeroLengthArrayArgument;
Add a few regexps about String#equals and toArray(new Object[0]) into checkstyle config file.

jihoonson · 2018-07-09T21:17:03Z

nit: Please make this final.

jihoonson · 2018-07-09T21:18:56Z

Can be Collections.singletonList.

jihoonson · 2018-07-09T21:23:59Z

Can be final.

jihoonson · 2018-07-09T21:24:29Z

Can be Collections.emptyList().

jihoonson · 2018-07-09T21:25:32Z

Please break the line per comma like below:

binder, Key.get(MetadataStorageConnectorConfig.class), new MetadataStorageConnectorConfig() { }

jihoonson · 2018-07-09T21:26:33Z

Can be final.

leventov · 2018-07-10T14:31:34Z

There is a corresponding inspection in IntelliJ, could you assign it ERROR severity (you could do it via IntelliJ interface; the change should appear in https://github.com/apache/incubator-druid/blob/master/.idea/inspectionProfiles/Druid.xml) so such problems are caught by CI.

Yes. Indeed, in this way, such a similar work will be more effective. By the way, can we learn from Alibaba's Java coding specification project P3C. Although the specifications in it are not completely correct... For example, i just submitted the issue about the pre-allocated size toArray method to P3C a few days ago. 😅

Already add ToArrayCallWithZeroLengthArrayArgument specification into profile.

leventov · 2018-07-10T14:36:12Z

Because, File.toURL() is deprecated.

Please add this method to our forbidden-apis file: https://github.com/apache/incubator-druid/blob/master/codestyle/druid-forbidden-apis.txt. When adding, please check that it works by intentionally leaving one call to this method in the code, and see if mvn clean install -DskipTests fails locally. For syntax, see https://github.com/policeman-tools/forbidden-apis/wiki/SignaturesSyntax.

leventov · 2018-07-10T14:38:34Z

Boolean.parseBoolean returns a primitive

There is no need for the .booleanValue() method, cuz Java will automatically unbox Boolean primitive after JDK 5+.

I mean there is no need to get a boxed Boolean either, Boolean.parseBoolean() is a direct equivalent which returns a primitive.

I see. I will fix it.

leventov · 2018-07-10T14:39:15Z

Also worth catching this consistently, via IntelliJ inspection

Yes, it is.

Original intellij inspection profile only catches obj != null && obj.equals("") situation, rather than obj.equals("")

After adding AliEqualsAvoidNull inspection into profile, it can recognizes obj.equals("") case

But, AliEqualsAvoidNull inspection need P3C plugin... What you think? @leventov

PS: The origin intellij inspection about string equals is SimplifiableEqualsExpression

I think you could catch those things merely using a regex, [a-z][a-zA-Z0-9_]*\.equals\((\"|[A-Z_]+\)) (intended to catch javaVar.equals("string") and javaVar.equals(STRING_CONSTANT)). You could add this regex to the checkstyle config: https://github.com/apache/incubator-druid/blob/master/codestyle/checkstyle.xml#L168

leventov · 2018-07-10T14:43:00Z

If you wish you could also fix all occurrences of this, via IntelliJ inspection "Too few arguments of Arrays.asList()", and prohibit it on CI level. There are hundreds of such issues in the codebase, but since there is an automatic fix in IntelliJ, it could be even not very hard to do this.

Okay, I will add this situation to the Intellij inspection configuration file. In this way, each newly added developer will notice this coding specification without having to read some sort of HOW TO CONTRIBUTE documents. 👍

Already add ArraysAsListWithZeroOrOneArgument specification into profile.

…eArgument into inspection profile

… type

asdf2014 · 2018-07-11T16:05:10Z

job-402722329 and job-402722330 both failed...

job-402722329:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.19.1:test (default-test) on project druid-processing: ExecutionException The forked VM terminated without properly saying goodbye. VM crash or System.exit called?
[ERROR] Command was /bin/sh -c cd /home/travis/build/apache/incubator-druid/processing && /usr/lib/jvm/java-8-oracle/jre/bin/java -Xmx768m -Duser.language=en -Duser.country=US -Dfile.encoding=UTF-8 -Duser.timezone=UTC -Djava.util.logging.manager=org.apache.logging.log4j.jul.LogManager -Ddruid.indexing.doubleStorage=double -jar /home/travis/build/apache/incubator-druid/processing/target/surefire/surefirebooter678641966742689650.jar /home/travis/build/apache/incubator-druid/processing/target/surefire/surefire1266228946009469426tmp /home/travis/build/apache/incubator-druid/processing/target/surefire/surefire_14722988346361641729tmp

It seems that -Xmx768m is not enough, may we should add -Xmx2048m -Xms512m for maven-surefire-plugin in druid-processing module?

job-402722330:

CuratorDruidCoordinatorTest.testMoveSegment:369 »  test timed out after 60000 ...

After PR#5978, sometimes the timeout value is still not enough.

…ease the timeout value for testMoveSegment testcase

jihoonson · 2018-07-11T18:22:59Z

The last commit is not related to this PR. Please make it as another one.

asdf2014 · 2018-07-12T00:25:31Z

Hi, @jihoonson . Already roll back the latest commit. I will create a independent PR for it. But, if the travis happens VM crash or testcase timeout cases again, please help me to rebuild the failed job. Thanks. 😃

jihoonson · 2018-07-12T00:30:03Z

@asdf2014 sure, I'll restart failed jobs if they're transient. BTW, the size of this PR looks suddenly increased. What commit has increased the size?

asdf2014 · 2018-07-12T00:32:34Z

These commits are a bit large.. 😅

asdf2014 · 2018-07-12T00:38:49Z

Under the advice of @leventov . After adding ToArrayCallWithZeroLengthArrayArgument & ArraysAsListWithZeroOrOneArgument & AliEqualsAvoidNull into intellij inspection profile, I automatically fix the entire project's coding inspection problems through the intellij idea tool.

jihoonson · 2018-07-12T00:38:58Z

@asdf2014 thanks. I'll take another look.

asdf2014 · 2018-07-12T00:44:41Z

@jihoonson You are welcome. The amount of code is large and very troublesome. May have to trouble you. 👍

asdf2014 · 2018-07-13T12:18:05Z

Hi, @jihoonson . jobs-403482349 failed again.. Could you please rebuild it?

jihoonson · 2018-07-13T17:37:23Z

@asdf2014 restarted the test. Will take a look again.

asdf2014 · 2018-07-13T17:39:55Z

@jihoonson Thanks a lot :D

asdf2014 · 2018-07-13T18:15:12Z

@jihoonson It works. Thx again. 👍

…pection profile

asdf2014 · 2018-07-17T02:28:39Z

@leventov PTAL.

Set the level of ArraysAsListWithZeroOrOneArgument inspection as warning, because use Collections.singletonList will cause a ClassCastException in io.druid.server.coordinator.CostBalancerStrategyBenchmark#factoryClasses and io.druid.collections.bitmap.WrappedRoaringBitmapTest#factoryClasses.
Also set the level of ToArrayCallWithZeroLengthArrayArgument coding inspection as warning and add BY_LEVEL option, because teamcity is misunderstanding this inspection, teamcity think pre-size way is better, but it is not.

@State(Scope.Thread)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@BenchmarkMode(Mode.AverageTime)
public class ToArrayBenchmark {

    @Param({"1", "100", "1000", "5000", "10000", "100000"})
    private int n;

    private final List<Object> list = new ArrayList<>();

    @Setup
    public void populateList() {
        for (int i = 0; i < n; i++) {
            list.add(0);
        }
    }

    @Benchmark
    public Object[] preSize() {
        return list.toArray(new Object[n]);
    }

    @Benchmark
    public Object[] resize() {
        return list.toArray(new Object[0]);
    }

    /*
    Integer List:
    Benchmark                    (n)  Mode  Cnt       Score        Error  Units
    ToArrayBenchmark.preSize       1  avgt    3      41.552 ±    108.030  ns/op
    ToArrayBenchmark.preSize     100  avgt    3     216.449 ±    799.501  ns/op
    ToArrayBenchmark.preSize    1000  avgt    3    2087.965 ±   6027.778  ns/op
    ToArrayBenchmark.preSize    5000  avgt    3    9098.358 ±  14603.493  ns/op
    ToArrayBenchmark.preSize   10000  avgt    3   24204.199 ± 121468.232  ns/op
    ToArrayBenchmark.preSize  100000  avgt    3  188183.618 ± 369455.090  ns/op
    ToArrayBenchmark.resize        1  avgt    3      18.987 ±     36.449  ns/op
    ToArrayBenchmark.resize      100  avgt    3     265.549 ±   1125.008  ns/op
    ToArrayBenchmark.resize     1000  avgt    3    1560.713 ±   2922.186  ns/op
    ToArrayBenchmark.resize     5000  avgt    3    7804.810 ±   8333.390  ns/op
    ToArrayBenchmark.resize    10000  avgt    3   24791.026 ±  78459.936  ns/op
    ToArrayBenchmark.resize   100000  avgt    3  158891.642 ±  56055.895  ns/op
    Object List:
    Benchmark                    (n)  Mode  Cnt      Score       Error  Units
    ToArrayBenchmark.preSize       1  avgt    3     36.306 ±    96.612  ns/op
    ToArrayBenchmark.preSize     100  avgt    3     52.372 ±    84.159  ns/op
    ToArrayBenchmark.preSize    1000  avgt    3    449.807 ±   215.692  ns/op
    ToArrayBenchmark.preSize    5000  avgt    3   2080.172 ±  2003.726  ns/op
    ToArrayBenchmark.preSize   10000  avgt    3   4657.937 ±  8432.624  ns/op
    ToArrayBenchmark.preSize  100000  avgt    3  51980.829 ± 46920.314  ns/op
    ToArrayBenchmark.resize        1  avgt    3     16.747 ±    85.131  ns/op
    ToArrayBenchmark.resize      100  avgt    3     43.803 ±    28.704  ns/op
    ToArrayBenchmark.resize     1000  avgt    3    404.681 ±   132.986  ns/op
    ToArrayBenchmark.resize     5000  avgt    3   1972.649 ±   174.691  ns/op
    ToArrayBenchmark.resize    10000  avgt    3   4021.440 ±  1114.212  ns/op
    ToArrayBenchmark.resize   100000  avgt    3  44204.167 ± 76714.850  ns/op
     */
    public static void main(String[] args) throws Exception {
        Options opt = new OptionsBuilder()
                .include(ToArrayBenchmark.class.getSimpleName())
                .forks(1)
                .warmupIterations(1)
                .measurementIterations(3)
                .threads(1)
                .build();
        new Runner(opt).run();
    }
}

Tips: Full code is here.

Added [a-z][a-zA-Z0-9_]*\.equals\((\"|[A-Z_]+\)) into checkstyle config file.
Added java.io.File#toURL() into druid-forbidden-apis.

leventov · 2018-07-17T17:27:43Z

@asdf2014

I don't know why CostBalancerStrategyBenchmark and WrappedRoaringBitmapTest are written in such an obscure way, you could fix the code to create arrays directly: new CostBalancerStrategy[] { new CostBalancerStrategy(MoreExecutors.listeningDecorator(Executors.newFixedThreadPool(1))) }, etc. and retain the inspection at the ERROR level.
It was fixed only recently: https://youtrack.jetbrains.com/issue/IDEA-184862, probably Youtrack didn't pick up the fix yet. I think you could also catch all such situations using a regex, something like toArray\([\s\n]*new [a-zA-Z0-9_]+\[[^0]

asdf2014 · 2018-07-18T15:36:01Z

Hi, @leventov . It has been fixed. BTW, jobs-405249198 failed because vm crashed. Would you please help me rebuild it once?

…ck the level of ToArrayCallWithZeroLengthArrayArgument as WARNING until Youtrack fix it

asdf2014 · 2018-07-19T14:42:19Z

@leventov It seems that both travis and teamcity have succeeded. Any other good suggestions?

leventov · 2018-07-20T23:27:28Z

      <property name="message" value="Use java.lang.Primitive.BYTES instead."/>
    </module>
+
+    <module name="Regexp">


Please add a comment that this regex should be replaced with an IntelliJ inspection when teamcity.jetbrains.com updates to at least IntelliJ 2018.1 (currently it uses 2017.2)

leventov · 2018-07-20T23:30:27Z

    Sequence<Result<TopNResultValue>> queryResult = theRunner.run(
        QueryPlus.wrap(query),
-        Maps.<String, Object>newHashMap()
+        Maps.newHashMap()


Did you remove redundant type arguments in the whole codebase? If so, you could try to change the level of the corresponding IntelliJ inspection ("Redundant type arguments") to ERROR.

leventov · 2018-07-20T23:32:20Z

  {
-    return Arrays.<AggregatorFactory>asList(new DistinctCountAggregatorFactory(fieldName, fieldName, bitMapFactory));
+    return Collections.singletonList(
+        new DistinctCountAggregatorFactory(fieldName, fieldName, bitMapFactory));


It's not formatted properly, the closing ); should be on the next line (or the whole expression on a single line).

leventov · 2018-07-20T23:32:55Z

  public List<AggregatorFactory> getRequiredColumns()
  {
-    return Arrays.<AggregatorFactory>asList(new TimestampAggregatorFactory(fieldName, fieldName, timeFormat, comparator, initValue));
+    return Collections.singletonList(


asdf2014 · 2018-07-21T03:36:01Z

Hi, @leventov . Thanks for your review. The RedundantTypeArguments has been added as an ERROR level check and teamcity has also been passed. PTAL.

leventov

Thanks a lot for this contribution, cleaning up the source is important but nobody usually does this.

asdf2014 · 2018-07-21T06:28:01Z

You are welcome. In the process, I also learned a lot. 👍

jihoonson · 2018-07-24T18:20:39Z

@asdf2014 would you update the PR description to include additional changes? E.g., Added new inspection/checkstyle rules.

asdf2014 · 2018-07-25T00:48:50Z

Hi, @jihoonson . Thanks for you comments. Added.

jihoonson · 2018-07-27T21:38:50Z

Thanks @asdf2014! The latest change looks good to me.

One side comment: usually, you can expect your PR can be reviewed faster as your PR contains less changes because it's much easier to review. It would be great if you can split your PRs if possible in the future.

asdf2014 · 2018-07-30T03:03:54Z

@jihoonson Thanks for your suggestion about the PR stuff. This is very helpful for me. I will notice it. 👍

asdf2014 mentioned this pull request Jul 9, 2018

Renamed 'Generic Column' -> 'Numeric Column'; Fixed a few resource leaks in processing; misc refinements #5957

Merged

jihoonson reviewed Jul 9, 2018

View reviewed changes

leventov reviewed Jul 10, 2018

View reviewed changes

asdf2014 changed the title ~~Various changes about druid-services module~~ Various changes about a few coding specifications Jul 11, 2018

asdf2014 added 12 commits July 11, 2018 20:46

Various changes about druid-services module

430f4d3

Patch improvements from reviewer

6d6ed05

Add ToArrayCallWithZeroLengthArrayArgument & ArraysAsListWithZeroOrOn…

5688fa2

…eArgument into inspection profile

Fix ArraysAsListWithZeroOrOneArgument

54e0295

Fix conflict

b3f1c81

Fix ToArrayCallWithZeroLengthArrayArgument

1b83c32

Fix AliEqualsAvoidNull

6d0d661

Remove blank line

0b69188

Remove unused import clauses

e26fdad

Fix code style in TopNQueryRunnerTest

9a8de71

Fix conflict

9df56bd

Don't use Collections.singletonList when converting the type of array…

def98c9

… type

Add argLine into maven-surefire-plugin in druid-process module & incr…

fcaf47d

…ease the timeout value for testMoveSegment testcase

Roll back the latest commit

07cca25

asdf2014 added 4 commits July 16, 2018 18:59

Fix style error for new regexp

7ffbe18

Set the level of ArraysAsListWithZeroOrOneArgument as WARNING

736c715

Fix style error for new regexp

d3a2d96

Add option BY_LEVEL for ToArrayCallWithZeroLengthArrayArgument in ins…

47a468d

…pection profile

asdf2014 added 2 commits July 18, 2018 15:21

Roll back the level as ToArrayCallWithZeroLengthArrayArgument as ERROR

e479086

Add toArray(new Object[0]) regexp into checkstyle config file & fix them

3af0a82

Set the level of ArraysAsListWithZeroOrOneArgument as ERROR & Roll ba…

32c39b8

…ck the level of ToArrayCallWithZeroLengthArrayArgument as WARNING until Youtrack fix it

leventov reviewed Jul 20, 2018

View reviewed changes

asdf2014 added 3 commits July 21, 2018 09:57

Add a comment for string equals regexp in checkstyle config

2e55e7a

Fix code format

ff5079c

Add RedundantTypeArguments as ERROR level inspection

4027b9c

leventov approved these changes Jul 21, 2018

View reviewed changes

leventov changed the title ~~Various changes about a few coding specifications~~ Remove redundant type parameters and enforce some other style and inspection rules Jul 21, 2018

asdf2014 and others added 2 commits July 25, 2018 14:09

Merge branch 'master' into various_changes

681ccf5

Fix cannot resolve symbol datasource

baa47dc

leventov mentioned this pull request Jul 25, 2018

Part 2 of changes for SQL Compatible Null Handling #5958

Merged

jihoonson approved these changes Jul 27, 2018

View reviewed changes

leventov merged commit 331a0af into apache:master Jul 27, 2018

asdf2014 deleted the various_changes branch July 30, 2018 03:03

dclim added this to the 0.13.0 milestone Oct 8, 2018

Conversation

asdf2014 commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asdf2014 commented Jul 11, 2018

Uh oh!

jihoonson commented Jul 11, 2018

Uh oh!

asdf2014 commented Jul 12, 2018

Uh oh!

jihoonson commented Jul 12, 2018

Uh oh!

asdf2014 commented Jul 12, 2018

Uh oh!

asdf2014 commented Jul 12, 2018

Uh oh!

jihoonson commented Jul 12, 2018

Uh oh!

asdf2014 commented Jul 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asdf2014 commented Jul 13, 2018

Uh oh!

jihoonson commented Jul 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

asdf2014 commented Jul 9, 2018 •

edited

Loading

asdf2014 commented Jul 12, 2018 •

edited

Loading

jihoonson commented Jul 13, 2018 •

edited

Loading

asdf2014 commented Jul 17, 2018 •

edited

Loading