[STORM-3037] Lowering CheckStyle Violations across all modules #2641

kishorvpatil · 2018-04-23T06:45:01Z

Below is the list of modules affected by this patch. The max allowed violations are going down by 17000+.

Module Name	Max Allowed Exceptions Before	Max Allowed Exceptions After
storm-client	10079	3298
storm-server	2585	783
storm-eventhubs	1765	45
storm-starter	1538	263
storm-hdfs	1406	189
storm-sql-core	1286	59
storm-redis	602	64
storm-cassandra	578	159
storm-kafka	557	180
storm-hbase	371	100
storm-maven-plugins	269	11
storm-hive	259	58
storm-core	254	73
storm-jms	235	63
storm-hdfs-examples	224	29
storm-kafka-monitor	178	87
storm-mqtt	158	39
storm-jdbc	149	36
storm-solr	108	47
storm-perf	100	65
storm-hbase-examples	55	16
maven-shade-clojure-transformer	16	0

Ethanlm · 2018-04-23T13:34:43Z

examples/storm-hdfs-examples/src/main/java/org/apache/storm/hdfs/bolt/SequenceFileTopology.java

                count = 0;
                System.out.println("Pending count: " + this.pending.size() + ", total: " + this.total);
            }
            Thread.yield();
        }

        public void ack(Object msgId) {
-//            System.out.println("ACK");
+            //            System.out.println("ACK");


Should we just remove this

This is attempt to only fix format to allow for checkstyle to pass it. I did not go into each comment for its validity for deletion.

Ethanlm

This is great work . But it's too much to review it at once. I will come back to review it later.

Ethanlm · 2018-04-23T13:35:25Z

...ples/storm-hbase-examples/src/main/java/org/apache/storm/hbase/topology/LookupWordCount.java

- * with the License.  You may obtain a copy of the License at
+ * 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


Why do we change this

The line length allowed is 140 characters. So the paragraphs are reformatted

Ethanlm · 2018-04-23T13:39:46Z

examples/storm-perf/src/main/java/org/apache/storm/perf/FileReadWordCountTopo.java

@@ -1,25 +1,24 @@
 /*


Should be /** here?

Ethanlm · 2018-04-23T13:43:42Z

examples/storm-perf/src/main/java/org/apache/storm/perf/JCQueuePerfTest.java

-//        threeProducer1Consumer(1);   // -- measurement 3
+        //        oneProducer1Consumer(1000);  // -- measurement 1
+        //        twoProducer1Consumer(1000);    // -- measurement 2
+        //        threeProducer1Consumer(1);   // -- measurement 3


Do we want to just remove these? Otherwise, delete the spaces between // and the actual code, e.g. //threeProducer1Consumer(1); to make it look better

Ethanlm · 2018-04-23T13:45:07Z

examples/storm-perf/src/main/java/org/apache/storm/perf/JCToolsPerfTest.java

+        //        twoProducer1Consumer();
+        //        threeProducer1Consumer();
+        //        oneProducer2Consumers();
+        //        producerFwdConsumer();


nit: spaces between // and code

Ethanlm · 2018-04-23T13:45:47Z

examples/storm-perf/src/main/java/org/apache/storm/perf/LowThroughputTopo.java

@@ -64,7 +63,7 @@ static StormTopology getTopology(Map<String, Object> conf) {
        BoltDeclarer bd = builder.setBolt(BOLT_ID, bolt, Helper.getInt(conf, BOLT_COUNT, 1));

        bd.localOrShuffleGrouping(SPOUT_ID);
-//        bd.shuffleGrouping(SPOUT_ID);
+        //        bd.shuffleGrouping(SPOUT_ID);


nit: space?

Ethanlm · 2018-04-23T13:49:07Z

examples/storm-perf/src/main/java/org/apache/storm/perf/spout/StringGenSpout.java

+        }
+        return result;
+    }
+


It looks like you just moved the location of the function. Is there any particular reason to do it?

Ethanlm · 2018-04-23T13:52:25Z

examples/storm-perf/src/main/java/org/apache/storm/perf/utils/BasicMetricsCollector.java

        String str = Utils.join(header, ",");
        writer.println(str);
-        writer.println("------------------------------------------------------------------------------------------------------------------");
+        writer
+            .println("------------------------------------------------------------------------------------------------------------------");


Would it be good to be consistent about this writer.println()? Here we have

writer.println( "\n---...

and

writer .println("---

Ethanlm · 2018-04-23T13:53:02Z

examples/storm-perf/src/main/java/org/apache/storm/perf/utils/MetricsSample.java

@@ -160,7 +159,7 @@ private static MetricsSample getMetricsSample(TopologyInfo topInfo) {
        ret.spoutEmitted = spoutEmitted;
        ret.spoutTransferred = spoutTransferred;
        ret.sampleTime = System.currentTimeMillis();
-//        ret.numSupervisors = clusterSummary.get_supervisors_size();
+        //        ret.numSupervisors = clusterSummary.get_supervisors_size();


remove this line ? or delete the space?

Ethanlm · 2018-04-23T13:54:22Z

examples/storm-starter/src/jvm/org/apache/storm/starter/BlobStoreAPIWordCountTopology.java

-            throw new RuntimeException(exp);
-        }
-    }
-


Why are we moving locations of the functions

There are checkstyle conditions that decide import order, modified overloaded method order and at-clause orders. To avoid these violations, we need to put these methods in right order.

revans2

+1 this is definitely a step in the right direction. It may not be perfect but there is more good in here.

Please upmerge as I merged one thing in that messed this up, sorry I'll try to hold off on more merges.

Also please respond to or address Ethan's concerns.

revans2 · 2018-04-26T20:12:47Z

Still +1

…-storm into STORM-3037 STORM-3037: Lowering CheckStyle Violations across all modules This closes #2641

kishorvpatil added 22 commits April 22, 2018 20:16

Fixing stylecheck problems with storm-client

8b34c6e

Fixing stylecheck problemswith storm-sql-core

1d8a9b6

Fixing stylecheck problemswith storm-server

f1c0bcb

Fixing stylecheck problems with storm-core

fc1cf09

Fixing stylecheck problems with storm-eventhubs

1872317

Fixing stylecheck problems with storm-hdfs

7da98cf

Fixing stylecheck problems with storm-starter

81ec15d

Fixing stylecheck problems with storm-redis

e3f5b13

Fixing stylecheck problems with storm-cassandra

1a2d131

Fixing stylecheck problems with storm-kafka

4fe4f04

Fixing stylecheck problems with storm-maven-plugins

84084ab

Fixing stylecheck problems with maven-shade-clojure-transformer

6ccf6a0

Fixing stylecheck problems with storm-hbase

880d14f

Fixing stylecheck problems with storm-hbase-examples

f4ba7c9

Fixing stylecheck problems with storm-perf

0e409ec

Fixing stylecheck problems with storm-hive

5fc4e9f

Fixing stylecheck problems with storm-kafka-monitor

224633d

Fixing stylecheck problems with storm-jms

95602b1

Fixing stylecheck problems with storm-solr

53adebc

Fixing stylecheck problems with storm-jdbc

6d20c4a

Fixing stylecheck problems with storm-mqtt

f7f3524

Fixing stylecheck problems with storm-hdfs-examples

e8ffac4

Ethanlm reviewed Apr 23, 2018

View reviewed changes

revans2 approved these changes Apr 23, 2018

View reviewed changes

kishorvpatil added 2 commits April 25, 2018 17:04

Merge branch 'master' of github.com:apache/storm into fix-style

99c62d4

Fix checkstyle issues

d9ca88a

asfgit merged commit d9ca88a into apache:master Apr 26, 2018

asfgit pushed a commit that referenced this pull request Apr 26, 2018

Merge branch 'fix-style' of https://github.com/kishorvpatil/incubator…

4137328

…-storm into STORM-3037 STORM-3037: Lowering CheckStyle Violations across all modules This closes #2641

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[STORM-3037] Lowering CheckStyle Violations across all modules #2641

[STORM-3037] Lowering CheckStyle Violations across all modules #2641

kishorvpatil commented Apr 23, 2018

Ethanlm Apr 23, 2018

kishorvpatil Apr 24, 2018

Ethanlm left a comment

Ethanlm Apr 23, 2018

kishorvpatil Apr 24, 2018

Ethanlm Apr 23, 2018

Ethanlm Apr 23, 2018

Ethanlm Apr 23, 2018

Ethanlm Apr 23, 2018

Ethanlm Apr 23, 2018

Ethanlm Apr 23, 2018

Ethanlm Apr 23, 2018

kishorvpatil Apr 26, 2018

Ethanlm Apr 23, 2018

kishorvpatil Apr 26, 2018 •

edited

revans2 left a comment

revans2 commented Apr 26, 2018

Navigation Menu

[STORM-3037] Lowering CheckStyle Violations across all modules #2641

[STORM-3037] Lowering CheckStyle Violations across all modules #2641

Conversation

kishorvpatil commented Apr 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ethanlm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishorvpatil Apr 26, 2018 • edited

Choose a reason for hiding this comment

revans2 left a comment

Choose a reason for hiding this comment

revans2 commented Apr 26, 2018

kishorvpatil Apr 26, 2018 •

edited