You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by curtisallen <gi...@git.apache.org> on 2014/12/24 00:22:14 UTC
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
GitHub user curtisallen opened a pull request:
https://github.com/apache/storm/pull/361
STORM-603 Log errors when required kafka params are missing
https://issues.apache.org/jira/browse/STORM-603
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.
```
[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
```
It took me a while to figure out that this error stems from missing the `backtype.storm.Config.STORM_ZOOKEEPER_CONNECTION_TIMEOUT` property in the `conf` map provided to the `storm.kafka.DynamicBrokersReader` constructer. It would be nice to check the required configuration parameters and throw a RuntimeException if any are missing.
This pull requests adds logging when required errors are missing.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/curtisallen/storm STORM-603
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/361.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #361
----
commit 326cc1c796d073c768de4b5aced1669bd47c6891
Author: Curtis Allen <cu...@pearson.com>
Date: 2014-12-23T23:13:31Z
Log errors when required kafka params are missing
----
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by revans2 <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/361#issuecomment-97525358
@curtisallen Thanks. If you could post a comment here when it is updated I'll take a look and we should hopefully be able to get it in soon.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by curtisallen <gi...@git.apache.org>.
Github user curtisallen commented on the pull request:
https://github.com/apache/storm/pull/361#issuecomment-97513938
@revans2 agreed, the concerns you brought make a lot of sense to me. I should have some time later this week to fail fast ;)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by curtisallen <gi...@git.apache.org>.
Github user curtisallen commented on the pull request:
https://github.com/apache/storm/pull/361#issuecomment-97934446
@revans2 now we throw NPEs if the values are missing. Leveraged guava's `Preconditions` . Let me know if you need anything else. Thanks!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/361#issuecomment-97201701
@curtisallen Sorry it has taken so long to review this. I think the changes look OK. I am curious why we only logging an error instead of throwing an exception. Wouldn't we just end up throwing an NPE later on in the code? Why not fail fast?
@harshach did @curtisallen answer all of your questions? Do you have any other follow up?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by curtisallen <gi...@git.apache.org>.
Github user curtisallen commented on a diff in the pull request:
https://github.com/apache/storm/pull/361#discussion_r22474024
--- Diff: external/storm-kafka/src/jvm/storm/kafka/DynamicBrokersReader.java ---
@@ -149,4 +158,19 @@ private Broker getBrokerHost(byte[] contents) {
}
}
+ private void validateConfig(final Map conf) {
+ if(conf.get(Config.STORM_ZOOKEEPER_SESSION_TIMEOUT) == null) {
--- End diff --
@harshach there are no defaults for these values, hence the reason for this pull request. The constructor for the DynamicBrokerReader accepts a `java.util.Map` Which must contain the values that are verified in this `validateConfig` method.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/361#discussion_r22435341
--- Diff: external/storm-kafka/src/jvm/storm/kafka/DynamicBrokersReader.java ---
@@ -41,6 +41,14 @@
private String _topic;
public DynamicBrokersReader(Map conf, String zkStr, String zkPath, String topic) {
+ // Check required parameters
+ if(conf == null) {LOG.error("conf cannot be null");}
--- End diff --
Instead putting it in DynamicBrokersReader it will be better if we have this in KafkaConfig and SpoutConfig and have a validate method there.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/361
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by revans2 <gi...@git.apache.org>.
Github user revans2 commented on the pull request:
https://github.com/apache/storm/pull/361#issuecomment-98820091
+1 looks great. Thanks for doing this
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by curtisallen <gi...@git.apache.org>.
Github user curtisallen commented on a diff in the pull request:
https://github.com/apache/storm/pull/361#discussion_r22474338
--- Diff: external/storm-kafka/src/jvm/storm/kafka/DynamicBrokersReader.java ---
@@ -41,6 +41,14 @@
private String _topic;
public DynamicBrokersReader(Map conf, String zkStr, String zkPath, String topic) {
+ // Check required parameters
+ if(conf == null) {LOG.error("conf cannot be null");}
--- End diff --
@harshach Agreed. I didn't take that approach because it would require significantly more changes to the spout since `DynamicBrokerReader` doesn't reference `KafkaConfig` or `SpoutConfig` currently. Also it would require a modification to `DynamicBrokerReader`'s constructor to accept a `KafkaConfig` instance breaking legacy code. I thought that would be a big change for a minor storm release.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by curtisallen <gi...@git.apache.org>.
Github user curtisallen commented on the pull request:
https://github.com/apache/storm/pull/361#issuecomment-97487810
@revans2 Thanks for looking at this. Before we didn't throw any exceptions when the config was incorrect. I added this [line](https://github.com/curtisallen/storm/blob/STORM-603/external/storm-kafka/src/jvm/storm/kafka/DynamicBrokersReader.java#L64) that throws an exception if the config is incorrect. Right now it's a RuntimeException however, would you prefer I throw a null pointer exception earlier?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] storm pull request: STORM-603 Log errors when required kafka param...
Posted by harshach <gi...@git.apache.org>.
Github user harshach commented on a diff in the pull request:
https://github.com/apache/storm/pull/361#discussion_r22435307
--- Diff: external/storm-kafka/src/jvm/storm/kafka/DynamicBrokersReader.java ---
@@ -149,4 +158,19 @@ private Broker getBrokerHost(byte[] contents) {
}
}
+ private void validateConfig(final Map conf) {
+ if(conf.get(Config.STORM_ZOOKEEPER_SESSION_TIMEOUT) == null) {
--- End diff --
These are all storm configs , even if user didn't pass these or set in storm.yaml there are defaults . So not sure checking for this necessary.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---