----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/53590/#review155930 -----------------------------------------------------------
Ship it! Overall this looks good to me, thanks for iterating! Mostly just some readability suggestions below on the docs. docs/features/job-updates.md (lines 52 - 53) <https://reviews.apache.org/r/53590/#comment226014> I know this is not your doing, but since you're in here... this part is not really accurate. The update is aborted based on the criteria specified in the UpdateConfig, not just some blanket percentage of health checks having failed. Mind updating? docs/features/services.md (lines 93 - 94) <https://reviews.apache.org/r/53590/#comment226015> "by introducing a `min_consecutive_successess` parameter .... This parameter represents the number of ..." docs/features/services.md (lines 95 - 96) <https://reviews.apache.org/r/53590/#comment226016> "Tasks that do not have enough successful health checks..." docs/features/services.md (line 102) <https://reviews.apache.org/r/53590/#comment226018> "...worst-case update time, it can instead be set to 0." src/main/python/apache/aurora/client/config.py (line 120) <https://reviews.apache.org/r/53590/#comment226021> given that the values being iterated are tuples and not key value pairs, probably better to explicitly name these vars. Something like `for value, name in params`? src/main/python/apache/aurora/executor/aurora_executor.py (lines 173 - 175) <https://reviews.apache.org/r/53590/#comment226023> indentation should be 4 spaces src/main/python/apache/aurora/executor/common/health_checker.py (line 180) <https://reviews.apache.org/r/53590/#comment226025> add a space after `attempt:` src/main/python/apache/aurora/executor/common/health_checker.py (line 201) <https://reviews.apache.org/r/53590/#comment226026> I can't think of any good reason not to keep incrementing attempts (e.g. numbers in Python don't overflow, memory utilization for even an exorbitantly large number is still trivial), but it stood out as a potential problem vector, so wanted to raise the question in case there's anything I'm missing: for a long lived service, is there any downside to continuing to increment attempts after the callback has been dispatched? - Joshua Cohen On Nov. 14, 2016, 5:09 a.m., Santhosh Kumar Shanmugham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/53590/ > ----------------------------------------------------------- > > (Updated Nov. 14, 2016, 5:09 a.m.) > > > Review request for Aurora, Joshua Cohen, Stephan Erb, and Zameer Manji. > > > Bugs: AURORA-1225 > https://issues.apache.org/jira/browse/AURORA-1225 > > > Repository: aurora > > > Description > ------- > > Make RUNNING a first class state to indicate that the task is running > and is healthy. It is achieved by introducing a new configuration > parameter `min_consecutive_successes`, which will dictate when to move > a task into RUNNING state. > > With this change, it is possible to set the `watch_secs` to 0, so that > updates are purely based on the task's health, rather than relying on > watching the task to in RUNNING state for a pre-determined timeout. > > > Diffs > ----- > > RELEASE-NOTES.md 392496284b7f046a5dd29a5a3f175305c28a5afe > docs/development/design-documents.md > 6bfc679c841ef1b0861758ccc12a5150f1bdb5e3 > docs/features/job-updates.md 792f2ae5fd14b1ea9af8be000629ce5a7fc2fe8f > docs/features/services.md b6bfc9de550a714b4243cf033e9f3f03ae676af2 > docs/reference/configuration.md f2a0b1873f31e91f3bf0cac6f8448e8130fae688 > docs/reference/task-lifecycle.md 4dcb48149e156ef1927e63f5a9fa6b53b174e755 > src/main/python/apache/aurora/client/api/updater_util.py > c649316edb876565c92cc90c9f030e153c008924 > src/main/python/apache/aurora/client/config.py > 0186af52f0d7d7e3981ec59bf6a01aafee2bcfb1 > src/main/python/apache/aurora/config/schema/base.py > 845163043b0b7b2f9e7aca14677ca9f094658551 > src/main/python/apache/aurora/executor/aurora_executor.py > aee5e56ae66ec7a6acbe97d94d6187fd8646ec9a > src/main/python/apache/aurora/executor/common/announcer.py > 12d8e0af95d4c24c1b7a3b07d01b59cec4a15894 > src/main/python/apache/aurora/executor/common/health_checker.py > 3c7c09d173c4adbd89eddaee22cffcdd8d268378 > src/main/python/apache/aurora/executor/common/resource_manager.py > b7dc40d8973ec2e5998ab4f6ff988051a70bb1ab > src/main/python/apache/aurora/executor/common/status_checker.py > 795dae2d6b661fc528d952c2315196d94127961f > src/test/python/apache/aurora/client/test_config.py > 5cf68a5145ddf9478baa30453c0bcb73136fa7eb > src/test/python/apache/aurora/executor/common/test_health_checker.py > da0c56ca084b65427a6122f22f251af8637772d1 > src/test/python/apache/aurora/executor/test_thermos_executor.py > 3f82165333665d127e1ace765a7dada7348a4b91 > src/test/sh/org/apache/aurora/e2e/http/http_example.aurora > c9dae28b83d751cfdc7c09dc2d83ac3ee4852720 > src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora > b85ace4bc33bda9483045732024302eb0725c89f > src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora > b3caa4186217dbf17282a5780791ce3157f4389b > src/test/sh/org/apache/aurora/e2e/http_example.py > 675ece85e2cf8e45f2c6e079e39b0e095075a3fa > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > 9cc6cec6633ee5973d8c9faa98ae24ab10cbeca4 > > Diff: https://reviews.apache.org/r/53590/diff/ > > > Testing > ------- > > buils-support/jenkins/build.sh > sh ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Santhosh Kumar Shanmugham > >
