----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36006/#review92625 -----------------------------------------------------------
samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 21) <https://reviews.apache.org/r/36006/#comment146833> We don't use camel style package names in Samza. It should be org.apache.samza.autoscaling.deployer samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 123) <https://reviews.apache.org/r/36006/#comment146838> Question: shouldn't the JobConfig be returned from the JobCoordinator's web interface? Or, more precisely, this ConfigManager should only need to know the CoordinatorStream topic to start with, since the JobCoordinator will send its url info into the CoordinatorStream topic. Even the initial config can be read from that topic as well. samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 131) <https://reviews.apache.org/r/36006/#comment146839> As Navina pointed out, we could have wrapped the Yarn related variables/functions into a YarnUtils class, which can hide all the details about yarnClient, hConfig, etc. samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 188) <https://reviews.apache.org/r/36006/#comment146841> Any reason the boostrap, skipUnreadMessages, readConfigMessages need to be public? samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 217) <https://reviews.apache.org/r/36006/#comment146842> The function name does not convey what it actually does. This is *not* just readConfigMessages, this method is *processing* messages in coordinator streams. I am still a bit confused among the three modes you defined here. If I read correctly, you are referring to three different *process_mode* here: 1) SKIP_ALL 2) PROCESS_SERVER_URL 3) PROCESS_CONTAINER_COUNT_AND_SERVER_URL Does the above make sense? I would suggest that you define a set-config message filter here that can define the list of messages that you want to react on. Therefore, if you want to skip all, the filter filters everything. And you can put arbitrary combination of different types of messages you want to react to, instead of just 3 different combinations here. samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 303) <https://reviews.apache.org/r/36006/#comment146843> Change to logger-based logging. And the message should be "killing the current job". samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 310) <https://reviews.apache.org/r/36006/#comment146844> And add a "Killed the current job" log line after confirming the job is dead. samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 318) <https://reviews.apache.org/r/36006/#comment146850> Did you update the container count in this config object? I couldn't find the code to update this before you start the JobRunner? samza-core/src/main/java/org/apache/samza/autoScaling/deployer/ConfigManager.java (line 338) <https://reviews.apache.org/r/36006/#comment146848> I don't see why jackson lib can't do this? Can we avoid adding the Gson dependency here? samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java (line 92) <https://reviews.apache.org/r/36006/#comment146851> How is it used? I don't see it used in ConfigManager. Where else could it be used? If no one uses it, we should remove this function. - Yi Pan (Data Infrastructure) 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 > >
