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

Remove redundant type parameters and enforce some other style and inspection rules #5980

Merged
merged 30 commits into from Jul 27, 2018

Conversation

Projects
None yet
4 participants
@asdf2014
Member

asdf2014 commented Jul 9, 2018

Various changes:

  1. Fix calling to toArray() with pre-sized array argument for better performance;
  2. Use lambda to complete the initialization of Module / Runnable / Accumulator;
  3. Remove unuesed import;
  4. Improve if-return clause;
  5. Use Set#addAll instead of for loop;
  6. Use Collections.<...>singletonList instead of Arrays.<...>asList for single element situation;
  7. Use file.toURI().toURL() instead of file.toURL();
  8. Remove unnecessary unboxing;
  9. Put the string object in front of the equals method;
  10. Use Collections.<StorageLocationConfig>emptyList() instead of Arrays.<StorageLocationConfig>asList();
  11. Add a few coding inspection rules, includes ArraysAsListWithZeroOrOneArgument, RedundantTypeArguments and ToArrayCallWithZeroLengthArrayArgument;
  12. Add a few regexps about String#equals and toArray(new Object[0]) into checkstyle config file.
@@ -116,7 +117,7 @@ private File makeIndexFiles(
List<Metadata> metadataList = Lists.transform(adapters, IndexableAdapter::getMetadata);
Metadata segmentMetadata = null;
Metadata segmentMetadata;

This comment has been minimized.

@jihoonson

jihoonson Jul 9, 2018

Contributor

nit: Please make this final.

@@ -776,7 +777,7 @@ public File persist(
log.info("Starting persist for interval[%s], rows[%,d]", dataInterval, index.size());
return merge(
Arrays.<IndexableAdapter>asList(
Collections.<IndexableAdapter>singletonList(

This comment has been minimized.

@jihoonson

jihoonson Jul 9, 2018

Contributor

Can be Collections.singletonList.

AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper);
// perform no-op authorization for these resources
AuthenticationUtils.addNoopAuthorizationFilters(root, UNSECURED_PATHS);
AuthenticationUtils.addNoopAuthorizationFilters(root, authConfig.getUnsecuredPaths());
authenticators = authenticatorMapper.getAuthenticatorChain();
List<Authenticator> authenticators = authenticatorMapper.getAuthenticatorChain();

This comment has been minimized.

@jihoonson

jihoonson Jul 9, 2018

Contributor

Can be final.

@@ -243,7 +243,7 @@ public void configure(Binder binder)
// configuration of other parameters, but I don't think that's actually a problem.
// Note, if that is actually not a problem, then that probably means we have the wrong abstraction.
binder.bind(SegmentLoaderConfig.class)
.toInstance(new SegmentLoaderConfig().withLocations(Arrays.<StorageLocationConfig>asList()));
.toInstance(new SegmentLoaderConfig().withLocations(Collections.<StorageLocationConfig>emptyList()));

This comment has been minimized.

@jihoonson

jihoonson Jul 9, 2018

Contributor

Can be Collections.emptyList().

binder, Key.get(MetadataStorageConnectorConfig.class), new MetadataStorageConnectorConfig()
binder -> {
JsonConfigProvider.bindInstance(
binder, Key.get(MetadataStorageConnectorConfig.class), new MetadataStorageConnectorConfig()

This comment has been minimized.

@jihoonson

jihoonson Jul 9, 2018

Contributor

Please break the line per comma like below:

binder, 
Key.get(MetadataStorageConnectorConfig.class), 
new MetadataStorageConnectorConfig()
{
}
AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper);
// perform no-op authorization for these resources
AuthenticationUtils.addNoopAuthorizationFilters(root, UNSECURED_PATHS);
AuthenticationUtils.addNoopAuthorizationFilters(root, authConfig.getUnsecuredPaths());
authenticators = authenticatorMapper.getAuthenticatorChain();
List<Authenticator> authenticators = authenticatorMapper.getAuthenticatorChain();

This comment has been minimized.

@jihoonson

jihoonson Jul 9, 2018

Contributor

Can be final.

Assert.assertTrue(dataSegment.getDimensions().size() == 1);
String[] dimensions = dataSegment.getDimensions().toArray(new String[dataSegment.getDimensions().size()]);
Assert.assertEquals(1, dataSegment.getDimensions().size());
String[] dimensions = dataSegment.getDimensions().toArray(new String[0]);

This comment has been minimized.

@leventov

leventov Jul 10, 2018

Member

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.

This comment has been minimized.

@asdf2014

asdf2014 Jul 10, 2018

Member

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. 😅

This comment has been minimized.

@asdf2014

asdf2014 Jul 11, 2018

Member

Already add ToArrayCallWithZeroLengthArrayArgument specification into profile.

@@ -454,8 +454,8 @@ public void testExtensionsWithSameDirName() throws Exception
final ClassLoader classLoader1 = Initialization.getClassLoaderForExtension(extension1, false);
final ClassLoader classLoader2 = Initialization.getClassLoaderForExtension(extension2, false);
Assert.assertArrayEquals(new URL[]{jar1.toURL()}, ((URLClassLoader) classLoader1).getURLs());
Assert.assertArrayEquals(new URL[]{jar2.toURL()}, ((URLClassLoader) classLoader2).getURLs());
Assert.assertArrayEquals(new URL[]{jar1.toURI().toURL()}, ((URLClassLoader) classLoader1).getURLs());

This comment has been minimized.

@leventov

leventov Jul 10, 2018

Member

Why this?

This comment has been minimized.

@asdf2014

asdf2014 Jul 10, 2018

Member

Because, File.toURL() is deprecated.

This comment has been minimized.

@leventov

leventov Jul 14, 2018

Member

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.

This comment has been minimized.

@asdf2014

asdf2014 Jul 16, 2018

Member

Added.

@@ -255,6 +255,6 @@ public LoadQueueTaskMaster getLoadQueueTaskMaster(
public static boolean isOverlord(Properties properties)
{
return Boolean.valueOf(properties.getProperty("druid.coordinator.asOverlord.enabled")).booleanValue();
return Boolean.valueOf(properties.getProperty("druid.coordinator.asOverlord.enabled"));

This comment has been minimized.

@leventov

leventov Jul 10, 2018

Member

Boolean.parseBoolean returns a primitive

This comment has been minimized.

@asdf2014

asdf2014 Jul 10, 2018

Member

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

This comment has been minimized.

@leventov

leventov Jul 14, 2018

Member

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

This comment has been minimized.

@asdf2014

asdf2014 Jul 14, 2018

Member

I see. I will fix it.

This comment has been minimized.

@asdf2014
@@ -150,7 +144,7 @@ public HadoopDruidIndexerConfig getHadoopDruidIndexerConfig()
final URI argumentSpecUri = new URI(argumentSpec);
final String argumentSpecScheme = argumentSpecUri.getScheme();
if (argumentSpecScheme == null || argumentSpecScheme.equals("file")) {
if (argumentSpecScheme == null || "file".equals(argumentSpecScheme)) {

This comment has been minimized.

@leventov

leventov Jul 10, 2018

Member

Also worth catching this consistently, via IntelliJ inspection

This comment has been minimized.

@asdf2014

asdf2014 Jul 10, 2018

Member

Yes, it is.

This comment has been minimized.

@asdf2014

asdf2014 Jul 11, 2018

Member

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

image

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

image

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

This comment has been minimized.

@asdf2014

asdf2014 Jul 11, 2018

Member

PS: The origin intellij inspection about string equals is SimplifiableEqualsExpression

This comment has been minimized.

@leventov

leventov Jul 14, 2018

Member

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

This comment has been minimized.

@asdf2014

asdf2014 Jul 16, 2018

Member

Added.

@@ -243,7 +242,7 @@ public void configure(Binder binder)
// configuration of other parameters, but I don't think that's actually a problem.
// Note, if that is actually not a problem, then that probably means we have the wrong abstraction.
binder.bind(SegmentLoaderConfig.class)
.toInstance(new SegmentLoaderConfig().withLocations(Arrays.<StorageLocationConfig>asList()));
.toInstance(new SegmentLoaderConfig().withLocations(Collections.emptyList()));

This comment has been minimized.

@leventov

leventov Jul 10, 2018

Member

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.

This comment has been minimized.

@asdf2014

asdf2014 Jul 10, 2018

Member

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. 👍

This comment has been minimized.

@asdf2014

asdf2014 Jul 11, 2018

Member

Already add ArraysAsListWithZeroOrOneArgument specification into profile.

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

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 11, 2018

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.

Add argLine into maven-surefire-plugin in druid-process module & incr…
…ease the timeout value for testMoveSegment testcase
@jihoonson

This comment has been minimized.

Contributor

jihoonson commented Jul 11, 2018

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

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 12, 2018

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

This comment has been minimized.

Contributor

jihoonson commented Jul 12, 2018

@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

This comment has been minimized.

Member

asdf2014 commented Jul 12, 2018

These commits are a bit large.. 😅

image

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 12, 2018

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

This comment has been minimized.

Contributor

jihoonson commented Jul 12, 2018

@asdf2014 thanks. I'll take another look.

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 12, 2018

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

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 13, 2018

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

@jihoonson

This comment has been minimized.

Contributor

jihoonson commented Jul 13, 2018

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

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 13, 2018

@jihoonson Thanks a lot :D

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 13, 2018

@jihoonson It works. Thx again. 👍

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 17, 2018

@leventov PTAL.

  1. 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.

  2. 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.

  1. Added [a-z][a-zA-Z0-9_]*\.equals\((\"|[A-Z_]+\)) into checkstyle config file.

  2. Added java.io.File#toURL() into druid-forbidden-apis.

@leventov

This comment has been minimized.

Member

leventov commented Jul 17, 2018

@asdf2014

  1. 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.

  2. 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

This comment has been minimized.

Member

asdf2014 commented Jul 18, 2018

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

Set the level of ArraysAsListWithZeroOrOneArgument as ERROR & Roll ba…
…ck the level of ToArrayCallWithZeroLengthArrayArgument as WARNING until Youtrack fix it
@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 19, 2018

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

@@ -171,5 +171,17 @@
<property name="illegalPattern" value="true"/>
<property name="message" value="Use java.lang.Primitive.BYTES instead."/>
</module>
<module name="Regexp">

This comment has been minimized.

@leventov

leventov Jul 20, 2018

Member

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)

This comment has been minimized.

@asdf2014

asdf2014 Jul 21, 2018

Member

Added.

@@ -372,7 +372,7 @@ public void queryMultiQueryableIndex(Blackhole blackhole)
Sequence<Result<TopNResultValue>> queryResult = theRunner.run(
QueryPlus.wrap(query),
Maps.<String, Object>newHashMap()
Maps.newHashMap()

This comment has been minimized.

@leventov

leventov Jul 20, 2018

Member

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.

This comment has been minimized.

@asdf2014

asdf2014 Jul 21, 2018

Member

Added.

@@ -137,7 +136,8 @@ public AggregatorFactory getCombiningFactory()
@Override
public List<AggregatorFactory> getRequiredColumns()
{
return Arrays.<AggregatorFactory>asList(new DistinctCountAggregatorFactory(fieldName, fieldName, bitMapFactory));
return Collections.singletonList(
new DistinctCountAggregatorFactory(fieldName, fieldName, bitMapFactory));

This comment has been minimized.

@leventov

leventov Jul 20, 2018

Member

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

This comment has been minimized.

@asdf2014

asdf2014 Jul 21, 2018

Member

Fixed.

@@ -148,7 +147,8 @@ public AggregatorFactory getMergingFactory(AggregatorFactory other) throws Aggre
@Override
public List<AggregatorFactory> getRequiredColumns()
{
return Arrays.<AggregatorFactory>asList(new TimestampAggregatorFactory(fieldName, fieldName, timeFormat, comparator, initValue));
return Collections.singletonList(

This comment has been minimized.

@leventov

This comment has been minimized.

@asdf2014

asdf2014 Jul 21, 2018

Member

Fixed.

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 21, 2018

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

This comment has been minimized.

Member

asdf2014 commented Jul 21, 2018

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

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

@jihoonson

This comment has been minimized.

Contributor

jihoonson commented Jul 24, 2018

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

@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 25, 2018

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

@jihoonson

This comment has been minimized.

Contributor

jihoonson commented Jul 27, 2018

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.

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@asdf2014

This comment has been minimized.

Member

asdf2014 commented Jul 30, 2018

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

@asdf2014 asdf2014 deleted the asdf2014:various_changes branch Jul 30, 2018

@dclim dclim added this to the 0.13.0 milestone Oct 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment