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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
