----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36006/#review94992 -----------------------------------------------------------
samza-autoscaling/src/main/java/org/apache/samza/autoscaling/deployer/ConfigManager.java (line 98) <https://reviews.apache.org/r/36006/#comment149763> The javadoc does not indicate that the polling Interval is configurable. samza-autoscaling/src/main/java/org/apache/samza/autoscaling/deployer/ConfigManager.java (line 289) <https://reviews.apache.org/r/36006/#comment149771> I was trying out your patch. 1000 seconds (~16 min) seems a litte too much time to wait. I would expect the overall time for wait (including multiple kill attempts) to be no more than 5 minutes. Can you change this hard-coded sleep time and make it configurable? samza-autoscaling/src/main/java/org/apache/samza/autoscaling/deployer/ConfigManager.java (line 291) <https://reviews.apache.org/r/36006/#comment149775> You are reading the application state only once outside the while loop in Line 286. So, the state never changes when the check happens in line 291. I think you should attempt to kill more than once and check the state each time. When I was trying out the patch, it killed the job fine. But it never came back up! samza-autoscaling/src/main/java/org/apache/samza/autoscaling/utils/YarnUtil.java (line 126) <https://reviews.apache.org/r/36006/#comment149741> Can you add a short description for these exceptions here to get rid of these warning? samza-autoscaling/src/main/java/org/apache/samza/autoscaling/deployer/ConfigManager.java:355: warning: no @param for args public static void main(String[] args) { ^ samza-autoscaling/src/main/java/org/apache/samza/autoscaling/utils/YarnUtil.java:126: warning: no description for @throws * @throws IOException ^ samza-autoscaling/src/main/java/org/apache/samza/autoscaling/utils/YarnUtil.java:127: warning: no description for @throws * @throws YarnException ^ - Navina Ramesh On Aug. 11, 2015, 6:38 p.m., Shadi A. Noghabi wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/36006/ > ----------------------------------------------------------- > > (Updated Aug. 11, 2015, 6:38 p.m.) > > > Review request for samza, Yi Pan (Data Infrastructure), Navina Ramesh, and > Naveen Somasundaram. > > > Repository: samza > > > Description > ------- > > This RBis for SAMZA-724. > > 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 a935088eccb3aee4fbde21275fa8e701c215a69e > checkstyle/import-control.xml 24ed680785175f3cdf955602b7a813684edd813e > gradle/dependency-versions.gradle fb06e8ed393d1a38abfa1a48fe5244fc7f6c7339 > > samza-autoscaling/src/main/java/org/apache/samza/autoscaling/deployer/ConfigManager.java > PRE-CREATION > > samza-autoscaling/src/main/java/org/apache/samza/autoscaling/utils/YarnUtil.java > PRE-CREATION > > samza-core/src/main/java/org/apache/samza/coordinator/stream/CoordinatorStreamSystemConsumer.java > 2277a732b9ab763edf19a0fbec288ff72b27583b > samza-core/src/main/scala/org/apache/samza/job/JobRunner.scala > d7c928c7401e539a370d4e82276e7dabbce1b638 > samza-shell/src/main/bash/run-config-manager.sh PRE-CREATION > > samza-test/src/test/scala/org/apache/samza/test/integration/StreamTaskTestUtil.scala > c6e994ff707af802ded57c3bc1762971892014da > > samza-yarn/src/main/scala/org/apache/samza/job/yarn/SamzaAppMasterService.scala > ce88698c12c4bf6f4cf128f92d60b0b9496997d7 > settings.gradle a8d2c885254ca3994327fda18e09c49bc9c5e830 > > Diff: https://reviews.apache.org/r/36006/diff/ > > > Testing > ------- > > Tested with hello samza and works properly. > > > Thanks, > > Shadi A. Noghabi > >
