[STORM-3282] Fix RAS worker count estimation#2902
Conversation
|
The jenkins failure seems unrelated. |
Ethanlm
left a comment
Ethanlm
left a comment
There was a problem hiding this comment.
Sorry my bad. Didn't know we have TOPOLOGY_WORKER_MAX_HEAP_SIZE_MB.
| return (int) Math.ceil(getEstimatedTotalHeapMemoryRequiredByTopo(topoConf, topology) / | ||
| ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB))); | ||
| Double defaultWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.WORKER_HEAP_MEMORY_MB)); | ||
| Double topologyWorkerMaxHeap = ObjectReader.getDouble(topoConf.get(Config.TOPOLOGY_WORKER_MAX_HEAP_SIZE_MB)); |
There was a problem hiding this comment.
Will topoConf.get(Config.WORKER_HEAP_MEMORY_MB) be null? If so, this will throw exception IllegalArgumentException("Don't know how to convert null to double");
There was a problem hiding this comment.
There is default of this value - https://github.com/apache/storm/blob/master/conf/defaults.yaml#L332 and https://github.com/apache/storm/blob/master/conf/defaults.yaml#L332 avoids it. But we can
So I don't anticipate that happening, but I am adding defaults.
There was a problem hiding this comment.
Oh. ok. If there is a default value in yaml file, then that's fine. I think we don't want to have to maintain two default values both in default.yaml and also in the code.
There was a problem hiding this comment.
The default in the code is last resort, as will be picked if we ever remove defaults from yaml
4 participants
No description provided.