----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36006/#review92227 -----------------------------------------------------------
Ideally, the job coordinator should act as the config manager. Since the chain of control is still from AppMaster to JobCoordinator (instead of the other way around), the only reasonable way to get this working is to have it as a separate script (like run-config-manager.sh). Having said this, I wonder if it is necessary for the ConfigManger to remain in samza-core. @Shadi: Were there any issues in keeping this code within the auto-scaling module? samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 47) <https://reviews.apache.org/r/36006/#comment146268> Remove commented lines samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 90) <https://reviews.apache.org/r/36006/#comment146269> RM_ADDRESS_OPT, RM_PORT_OPT, POLLING_INTERVAL_OPT, SERVER_URL_OPT can be made private samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 151) <https://reviews.apache.org/r/36006/#comment146272> Use logger instance samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 215) <https://reviews.apache.org/r/36006/#comment146271> nit: typo in "server ural" samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 236) <https://reviews.apache.org/r/36006/#comment146381> We should, in the future, change the handlers to implement a common interface. This way, users can register custom handlers and also, perhaps precondition checks. Whether that will open up to mishandling of config changes is still up to debate. Just wanted to comment this here. samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 310) <https://reviews.apache.org/r/36006/#comment146679> Can you add a check here to verify that the job ha actually been killed? You can use the rest api to verify that the status of the job has been changed to "KILLED". samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 317) <https://reviews.apache.org/r/36006/#comment146680> I think it is fine to skip messages written to the stream after you have killed a job's application attempt and before the next attempt starts up. However you can't avoid the bootstrap code from reading these messages. I don't think there is clean solution for this here. samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 330) <https://reviews.apache.org/r/36006/#comment146682> Can you move the yarn rest api related code to a separate YarnUtils class? Makes the code more cleaner and it can be re-used in other places in the future. samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 392) <https://reviews.apache.org/r/36006/#comment146684> Why do you need this class here? samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala (line 52) <https://reviews.apache.org/r/36006/#comment146685> Can you please add a comment here for what "isFirstTime" is used for? samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala (line 123) <https://reviews.apache.org/r/36006/#comment146687> Code here looks kind of arbitary here. Looks like you want to write the jobcoordinator url to the coordinator stream. You should perhaps move this to the init method in SamzaAppMasterService?? This way you don't have to change the interface of the run method. - Navina Ramesh On July 18, 2015, 2:52 a.m., Shadi A. Noghabi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36006/ > ----------------------------------------------------------- > > (Updated July 18, 2015, 2:52 a.m.) > > > Review request for samza, Yi Pan (Data Infrastructure), Navina Ramesh, and > Naveen Somasundaram. > > > Repository: samza > > > Description > ------- > > After a job is submitted, it might need some configuration change, > specifically it might need more containers. In SAMZA-704 a tool is being > added to write to the coordinator stream (CoordinatorStreamWriter). This > tool can be used to write new configurations to the coordinator stream. > However, another tool (ConfigManager) is needed to read the config changes > and react to them, which is the goal of this task. This tool should be > brought up after the job is submitted and read any config changes added to > the coordinator stream, and react to each accordingly. > > This tool, called the Config Manager, is focusing on handling container > changs by reacting to set-config massages with key "yarn.container.count". > > The config manager is a separate standa alone module, that should be brought > up separately after the submission of a job. Therefore, you have to add two > configurations to the input config file: > 1. yarn.rm.address= <ip of resource manager in yarn. ex: localhost > > 2. yarn.rm.port= <the port of the resource manager http server. ex: 8088 > > > The config manger will periodically poll the coordinator stream to see if > there are any new messages. This period is set to 100 ms by deafualt. > However, it can be configured by adding > configManager.polling.interval=<polling interval> to the input config file. > Thus, overal the command to run the config manager along with the job would > be: > > > <path to samza deployment>/bin/run-config-manager.sh --config-factory=<config > factory> --config-path=<path to config file of a job> > > > Diffs > ----- > > build.gradle 0852adc4e8e0c2816afd1ebf433f1af6b44852f7 > checkstyle/import-control.xml 6654319392929857bb861d77763afd8a5ea7674c > gradle/dependency-versions.gradle fb06e8ed393d1a38abfa1a48fe5244fc7f6c7339 > > samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java > b1078bdf7bddd16c9ccc6559b9efd40ca5ae67bc > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > 1c178a661e449c6bdfc4ce431aef9bb2d261a6c2 > samza-shell/src/main/bash/run-config-manager.sh PRE-CREATION > > samza-test/src/test/scala/org/apache/samza/test/integration/TestStatefulTask.scala > ea702a919348305ff95ce0b4ca1996a13aff04ec > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMaster.scala > af42c6a6636953a95f79837fe372e0dbd735df70 > > samza-yarn/src/test/scala/org/apache/samza/job/yarn/TestSamzaAppMaster.scala > 7b7d86a43c69e72c47eaa91f68be24e0f4022891 > > Diff: https://reviews.apache.org/r/36006/diff/ > > > Testing > ------- > > > Thanks, > > Shadi A. Noghabi > >
