Change YarnKeys to enum #1707
Change YarnKeys to enum #1707
Conversation
| // yarn queue for submitting and launching the topology | ||
| HERON_SCHEDULER_YARN_QUEUE("heron.scheduler.yarn.queue", "default"), | ||
| // the amount of memory topology's driver (yarn application master) needs | ||
| YARN_SCHEDULER_DRIVER_MEMORY_MB("heron.scheduler.yarn.driver.memory.mb", 2048); |
There was a problem hiding this comment.
Instead of using int, can we change this to ByteAmount and drop '.mb' from the key value (see https://github.com/twitter/heron/blob/master/heron/spi/src/java/com/twitter/heron/spi/common/Key.java#L104)? We could then use ByteAmount everywhere and then call asMegabytes before adding it to the yarn config, which I suspect requires MBs.
If this would trigger a large refactor, I'm fine with shipping this as-is and following up with the ByteAmount change in it's own PR.
There was a problem hiding this comment.
@billonahill - Thanks for reviewing. It will be great if we change driver.memory.mb from int to ByteAmount. I would like to send another PR for this. BTW, dropping '.mb' might be not good for users cause they need to know the unit of this parameter.
Not sure if necessary. As PR #1693 refactored, I'm doing the same thing on YARN-Scheduler, replacing Yarn config key from
STATIC STRINGtoENUM.The key changes are following:
ENUMin YarnContext classENUMin YarnLauncherTestcc @billonahill @ashvina