[STORM-3004] storm-elasticsearch-examples: fixed all checkstyle warnings #2596

krichter722 · 2018-03-19T00:35:12Z

I'd like to share my hands-on reflection on whether it makes sense to take over you checkstyle-config for some of my projects. The approx. 100 violations cost about 1,5h to fix which seems a realistic indicator for the time needed for similar fixes.

srdo · 2018-03-23T17:17:30Z

@krichter722 Thanks for the fixes. I'll review this as soon as I can. In the meantime, could you raise issues on https://issues.apache.org/jira to track your changes? It makes it easier for us to track which branches fixes have been applied to, and we use the JIRA issues to generate release notes. Please also rename the PR and commit to contain the JIRA issue number. Thanks :)

srdo

Looks pretty good, left some comments. My only real concern is that I feel like some of the comments don't really help understand the code, because they're documenting e.g. field X with "The X", which seems redundant.

srdo · 2018-03-24T12:25:03Z

...lasticsearch-examples/src/main/java/org/apache/storm/elasticsearch/bolt/EsIndexTopology.java

@@ -34,52 +39,128 @@
 import org.apache.storm.tuple.Fields;
 import org.apache.storm.tuple.Values;

-public class EsIndexTopology {
+/**
+ * Demonstrates an ElasticSearch Strom topology.


Strom -> Storm

srdo · 2018-03-24T12:26:04Z

...lasticsearch-examples/src/main/java/org/apache/storm/elasticsearch/bolt/EsIndexTopology.java

-public class EsIndexTopology {
+/**
+ * Demonstrates an ElasticSearch Strom topology.
+ * @author unknown


We generally don't use @author in the javadoc, it's just noise since most files are worked on by more than one person. Could you remove it here and elsewhere?

srdo · 2018-03-24T12:27:03Z

...lasticsearch-examples/src/main/java/org/apache/storm/elasticsearch/bolt/EsIndexTopology.java


+    /**
+     * The id of the used spout.
+     */
    static final String SPOUT_ID = "spout";


I think these should be private, that way checkstyle hopefully won't complain about missing comments.

srdo · 2018-03-24T12:29:16Z

...lasticsearch-examples/src/main/java/org/apache/storm/elasticsearch/bolt/EsIndexTopology.java

        };
+        /**


Is checkstyle requiring these comments? If not I'd rather we don't have them. Comments that are just "The $variable_name" are just noise.

Yes, the current setup requires comments on almost everything. I agree that they almost no use. Therefore this PR is a good usability check for the current extremely strict checkstyle configuration.

I checked, and checkstyle passes fine for me without these comments. As far as I can tell, comments are required on public classes, public methods that don't override supertype methods, and public constructors/variables.

For example, this file works for me with only comments on the EsIndexTopology class and the main method. All the others can be removed without violating the checkstyle check.

Does this not work for you?

Ah, you're right, I was using the checkstyle:check target which apparently uses a much stricter set of rules which might be even different from the one configured to run in the validate phase. I'll remove the noisy comments on private fields and methods.

srdo · 2018-03-24T12:31:05Z

...lasticsearch-examples/src/main/java/org/apache/storm/elasticsearch/bolt/EsIndexTopology.java

        }

+        /**
+         * Processes the next tuple.


Nit: nextTuple makes the spout emit the next tuple, if any.

srdo · 2018-03-24T12:40:06Z

...csearch-examples/src/main/java/org/apache/storm/elasticsearch/trident/TridentEsTopology.java

+ */
+public final class TridentEsTopology {
+    /**
+     * The default batch size. Necessary in order to avoid magic number


Nit: I don't think you need to mention why the field is here. Won't checkstyle complain if someone replaces the field with a magic number again?

srdo · 2018-03-24T12:41:34Z

...csearch-examples/src/main/java/org/apache/storm/elasticsearch/trident/TridentEsTopology.java

        @Override
        public Fields getOutputFields() {
            return new Fields("source", "index", "type", "id");
        }

+        /**
+         * Open the topology.


Nit: Opens the spout, not the topology

srdo · 2018-03-24T12:42:39Z

...csearch-examples/src/main/java/org/apache/storm/elasticsearch/trident/TridentEsTopology.java

            }
            for (List<Object> list : batch) {
                collector.emit(list);
            }
        }

+        /**
+         * Acknoledges the message with id {@code msgId}.


Acknowledges

srdo · 2018-03-24T12:43:10Z

...csearch-examples/src/main/java/org/apache/storm/elasticsearch/trident/TridentEsTopology.java

            if (batch == null) {
                batch = new ArrayList<List<Object>>();
-                if (index >= outputs.length && cycle) {
+                if (index >= outputs.length && isCycle()) {


Nit: Doesn't seem like there's any reason to use the getter rather than referring to the field?

srdo · 2018-03-24T12:44:14Z

...csearch-examples/src/main/java/org/apache/storm/elasticsearch/trident/TridentEsTopology.java

+         * Get the maximum batch size.
+         * @return the maxBatchSize
+         */
+        public int getMaxBatchSize() {


A little unsure why we need these getters and setters?

krichter722 · 2018-03-25T15:18:12Z

I agree with all your comments and removed the TridentEsTopology.get/setMaxBatchSize. The noisy comments are required (see comment above). Thanks for the review.

srdo

Looks good pending the last two comments, +1 after that.

srdo · 2018-03-26T16:31:04Z

external/storm-opentsdb/pom.xml

@@ -1,3 +1,4 @@
+


This change seems unnecessary.

srdo · 2018-03-26T16:32:19Z

...lasticsearch-examples/src/main/java/org/apache/storm/elasticsearch/bolt/EsIndexTopology.java

            System.out.println("**** RESENDING FAILED TUPLE");
            this.collector.emit(this.pending.get(msgId), msgId);
        }
    }
+
+    /**
+     * Utility constructor.


Thanks for the explanation, that makes sense. Could you change the comment to e.g. "Prevent instantiation" or something similar, so people who run across this later will know why it's there?

HeartSaVioR · 2018-03-28T11:17:23Z

@krichter722 kindly reminder.

krichter722 · 2018-03-28T11:54:24Z

Last comments incorporated.

srdo · 2018-03-28T19:45:24Z

+1, thanks for being patient. @krichter722 could you add the issue number to the commit message, I'll merge after that. I'd add it myself, but then the PR won't show as merged.

srdo · 2018-03-28T20:07:59Z

@krichter722 Thanks, merged to master. I also added you to the contributors list in JIRA, so you should be able to assign tasks to yourself now.

krichter722 changed the title ~~storm-elasticsearch-examples: fixed all checkstyle warnings~~ [STORM-3004] storm-elasticsearch-examples: fixed all checkstyle warnings Mar 24, 2018

srdo reviewed Mar 24, 2018

View reviewed changes

srdo reviewed Mar 26, 2018

View reviewed changes

[STORM-3004] storm-elasticsearch-examples: fixed all checkstyle warnings

eb72603

asfgit merged commit eb72603 into apache:master Mar 28, 2018

krichter722 deleted the checkstyle-fix-es-examples branch May 12, 2018 21:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[STORM-3004] storm-elasticsearch-examples: fixed all checkstyle warnings #2596

[STORM-3004] storm-elasticsearch-examples: fixed all checkstyle warnings #2596

krichter722 commented Mar 19, 2018 •

edited

Loading

srdo commented Mar 23, 2018

srdo left a comment

srdo Mar 24, 2018

srdo Mar 24, 2018

srdo Mar 24, 2018

srdo Mar 24, 2018

krichter722 Mar 25, 2018

srdo Mar 25, 2018

krichter722 Mar 26, 2018

srdo Mar 24, 2018

srdo Mar 24, 2018

srdo Mar 24, 2018

srdo Mar 24, 2018

srdo Mar 24, 2018

srdo Mar 24, 2018

krichter722 commented Mar 25, 2018

srdo left a comment

srdo Mar 26, 2018

srdo Mar 26, 2018

HeartSaVioR commented Mar 28, 2018

krichter722 commented Mar 28, 2018

srdo commented Mar 28, 2018

srdo commented Mar 28, 2018

[STORM-3004] storm-elasticsearch-examples: fixed all checkstyle warnings #2596

[STORM-3004] storm-elasticsearch-examples: fixed all checkstyle warnings #2596

Conversation

krichter722 commented Mar 19, 2018 • edited Loading

srdo commented Mar 23, 2018

srdo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krichter722 commented Mar 25, 2018

srdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeartSaVioR commented Mar 28, 2018

krichter722 commented Mar 28, 2018

srdo commented Mar 28, 2018

srdo commented Mar 28, 2018

krichter722 commented Mar 19, 2018 •

edited

Loading