Github user knusbaum commented on a diff in the pull request:
https://github.com/apache/storm/pull/921#discussion_r47833630
--- Diff: storm-core/src/jvm/backtype/storm/scheduler/TopologyDetails.java
---
@@ -396,34 +410,50 @@ public void addResourcesForExec(ExecutorDetails exec,
Map<String, Double> resour
LOG.warn("Executor {} already exists...ResourceList: {}",
exec, getTaskResourceReqList(exec));
return;
}
- _resourceList.put(exec, resourceList);
+ this.resourceList.put(exec, resourceList);
}
/**
* Add default resource requirements for a executor
*/
public void addDefaultResforExec(ExecutorDetails exec) {
+ Double topologyComponentCpuPcorePercent =
Utils.getDouble(this.topologyConf.get(Config.TOPOLOGY_COMPONENT_CPU_PCORE_PERCENT),
null);
+ if (topologyComponentCpuPcorePercent == null) {
+ LOG.warn("default value for " +
Config.TOPOLOGY_COMPONENT_CPU_PCORE_PERCENT + " needs to be set!");
+ }
--- End diff --
This will **only** occur if a developer has deleted an existing value in
defaults.yaml - that is, they are working on Storm itself, not building a
topology. The topology config validation will take care of that - There is no
way a user can submit a topology that will cause this on a properly compiled
Storm cluster.
This is determined at compile-time, and will be caught by unit tests.
Having it caught in unit tests is preferable since it will raise an alert on
any pull requests that break it, whereas having it here won't raise an alert
until a cluster is up and someone tries to launch something.
I can still see the usefulness of these checks IF we throw
RuntimeExceptions here for two main reasons:
1. It might blow up later, and when I look at the logs and see a stack
trace, I don't go back too far to look for other stuff. So if we don't throw
here, the users are going to be dealing with the same NPE in a weird spot you
were trying to avoid.
2. It might *not* blow up later, and instead just exhibit some weird
behavior. This will be worse because there won't be a stack trace for the user
to find, and they won't find any errors in the logs. (These are `LOG.warn()`
calls)
---
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.
---
