[
https://issues.apache.org/jira/browse/STORM-603?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14519779#comment-14519779
]
ASF GitHub Bot commented on STORM-603:
--------------------------------------
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/361#issuecomment-97512345
@curtisallen I am not really concerned about it being an NPE, or a
RuntimeException, my concern is two fold.
First If we mis configure something like Config we log
[something](https://github.com/curtisallen/storm/blob/STORM-603/external/storm-kafka/src/jvm/storm/kafka/DynamicBrokersReader.java#L45)
it and then later end up throwing a RuntimeException. In this case the
RuntimeException is wrapping an NPE that will only have the line number of
where the NPE happened. If all we had were the logs to go off of then it would
not be a big deal, but in storm the exception and the stack trace often are
returned and displayed on the UI. I would much rather see "Config cannot be
null" on the UI instead of NPE on line 162 of DynamicBrokersReader.
My second concern is around failing fast. In the case of _zkPath or _topic
being null. There is nothing that explicitly uses them in the constructor.
The failure will happen much later if ever as is the case for _zkPath and
_topic, as they appear to only be used as a part of string concatenation with a
+. Which when done with a null writes "null" to the string instead of throwing
an NPE, so we could end up trying to read from "/kafka/topics/null/partitions".
If something is wrong in a way that we cannot recover from I would much
rather throw an exception the moment we know it is wrong rather than log an
error and hope that the exception will be thrown later on, or that somehow we
will manage to not actually need this config to be set.
> storm-kafka: Log errors when missing required configuration fields
> ------------------------------------------------------------------
>
> Key: STORM-603
> URL: https://issues.apache.org/jira/browse/STORM-603
> Project: Apache Storm
> Issue Type: Bug
> Reporter: Curtis Allen
> Assignee: Curtis Allen
>
> I was upgrading our topologies to storm-0.9.3 this
> [commit|https://github.com/apache/storm/commit/2596e335f27a57784a93a57823bd93dde587909f]
> introduced a change that threw me for a loop. When submitting my topology I
> got the following error.
> {code}
> [main] ERROR storm.kafka.DynamicBrokersReader - Couldn't connect to zookeeper
> java.lang.IllegalArgumentException: Don't know how to convert null to int
> at backtype.storm.utils.Utils.getInt(Utils.java:301)
> ~[storm-core-0.9.3.jar:0.9.3]
> at
> storm.kafka.DynamicBrokersReader.<init>(DynamicBrokersReader.java:47)
> ~[gambit-storm-threads-0.0.1-SNAPSHOT-jar-with-dependencies.jar:na]
> at
> com.pearson.gambit.threads.storm.ThreadsTopology.main(ThreadsTopology.java:45)
> [gambit-storm-threads-0.0.1-SNAPSHOT-jar-with-dependencies.jar:na]
> Exception in thread "main" java.lang.RuntimeException:
> java.lang.RuntimeException: java.lang.NullPointerException
> at
> storm.kafka.DynamicBrokersReader.getBrokerInfo(DynamicBrokersReader.java:81)
> at
> com.pearson.gambit.threads.storm.ThreadsTopology.main(ThreadsTopology.java:48)
> Caused by: java.lang.RuntimeException: java.lang.NullPointerException
> at
> storm.kafka.DynamicBrokersReader.getNumPartitions(DynamicBrokersReader.java:94)
> at
> storm.kafka.DynamicBrokersReader.getBrokerInfo(DynamicBrokersReader.java:65)
> ... 1 more
> Caused by: java.lang.NullPointerException
> at
> storm.kafka.DynamicBrokersReader.getNumPartitions(DynamicBrokersReader.java:91)
> ... 2 more
> {code}
> It took me a while to figure out that this error stems from missing the
> {code}backtype.storm.Config.STORM_ZOOKEEPER_CONNECTION_TIMEOUT{code} property
> in the conf map provided to the {code}storm.kafka.DynamicBrokersReader{code}
> constructer. It would be nice to check the required configuration parameters
> and throw a RuntimeException if any are missing.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
