> On May 16, 2018, 2:35 p.m., Stephan Erb wrote: > > I have done a first quick pass. I will have a second closer look once the > > storage patch has landed.
Thanks for the review. Much appreciated. > On May 16, 2018, 2:35 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > > Lines 74 (patched) > > <https://reviews.apache.org/r/66716/diff/7/?file=2023157#file2023157line74> > > > > I think we should make this value configurable for operators. Done. > On May 16, 2018, 2:35 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > > Lines 248-259 (patched) > > <https://reviews.apache.org/r/66716/diff/7/?file=2023157#file2023157line248> > > > > Both this validation and the other `instance`-based validation below > > have a conceptual problem: Users can freely adjust the number of available > > instances via `kill` and `addInstance`. This implies that the policy can > > become invalid long after we have checked it here. > > > > I think the most robust way would be if we just print warnings in the > > client, and change the scheduler so that it can ignore invalid policies, > > similar to how it ignores them after a timeout. Good catch. I will add the warning and the validation check. > On May 16, 2018, 2:35 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > > Lines 276 (patched) > > <https://reviews.apache.org/r/66716/diff/7/?file=2023157#file2023157line276> > > > > `"CountSlaPolicy: count=5 must be less than 3"` can be hard to > > understand as it lacks context. > > > > If you add that the second number refers to the number of instances, it > > it will be easier to users to reason about the error. Done. > On May 16, 2018, 2:35 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java > > Lines 280 (patched) > > <https://reviews.apache.org/r/66716/diff/7/?file=2023160#file2023160line280> > > > > Would you be open to the idea of passing the host and (health?) port of > > the instance here as well? I have a usecase in mind that would be > > simplified by this quite a bit as I can query a running instance for > > additional information without having to query the the service discover ZK > > first. > > > > In addition, have you considered just passing each information > > individually (job, environment, role,...) rather than as a joined string? > > Many usescases will probably require string splitting within the > > coordinator otherwise. Adding the entire `ScheduledTask` into the body in JSON format, similar to `WebHooks` (but unlike that we will use `TSerializer` to cleanly serialize to JSON). > On May 16, 2018, 2:35 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java > > Lines 303 (patched) > > <https://reviews.apache.org/r/66716/diff/7/?file=2023160#file2023160line303> > > > > What usecase do you have in mind for the statuskey? > > > > I am mostly wondering if a protocol purely based on HTTP status codes > > would be sufficient: e.g. `200 OK` means ready to drain and `428 > > PRECONDITION REQUIRED` asks us to come back again later. I foresee this becoming a for full-fledged protocol with its own library in future depending on the expectations from more stateful services. Some of the possible usecases are "come back after x mins from now", "add instance before killing". > On May 16, 2018, 2:35 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java > > Lines 384 (patched) > > <https://reviews.apache.org/r/66716/diff/7/?file=2023160#file2023160line384> > > > > isProduction is deprecated. You will need to check the appropriate tier > > config here. Done. - Santhosh Kumar ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66716/#review203276 ----------------------------------------------------------- On May 17, 2018, 7:49 p.m., Santhosh Kumar Shanmugham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66716/ > ----------------------------------------------------------- > > (Updated May 17, 2018, 7:49 p.m.) > > > Review request for Aurora, David McLaughlin, Jordan Ly, and Stephan Erb. > > > Repository: aurora > > > Description > ------- > > `Tasks` can specify custom SLA requirements as part of > their `TaskConfig`. One of the new features is the ability > to specify an external coordinator that can ACK/NACK > maintenance requests for tasks. This will be hugely > beneficial for onboarding services that cannot satisfactorily > specify SLA in terms of running instances. > > Maintenance requests are driven from the Scheduler to > improve management of nodes in the cluster. > > > Diffs > ----- > > RELEASE-NOTES.md 5e1f9940a7974e212140b7e5304695afa7f96e78 > api/src/main/thrift/org/apache/aurora/gen/api.thrift > ef754e32172e7490a47a13e7b526f243ffa3efeb > api/src/main/thrift/org/apache/aurora/gen/storage.thrift > b79e2045ccda05d5058565f81988dfe33feea8f1 > docs/README.md 166bf1ce240474f0a181e023439cfbfbe7363822 > docs/features/sla-requirements.md PRE-CREATION > docs/reference/configuration.md d4b869b938105ba301fc88d41019af2f1707f6f4 > docs/reference/scheduler-configuration.md > a659cfac974059b04ef5593286011decbb7f9110 > src/main/java/org/apache/aurora/scheduler/app/AppModule.java > ffc07443fae9e5216a5333ae305f75aa9b452a0c > src/main/java/org/apache/aurora/scheduler/config/CliOptions.java > a2fb0393ba47e876c4c8c63e3ed27ebe42cb6ca3 > > src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java > 4073229b74d0e0e7fd31552bd96894ceb8a0971a > > src/main/java/org/apache/aurora/scheduler/maintenance/MaintenanceModule.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/mesos/MesosCallbackHandler.java > 3b4df55a05873e79aae206b117cbc753fa3abb94 > src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/sla/SlaModule.java > 25ed474289f369e74c24e999ad97ed6810c9fd5e > src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java > f58c66aaebe8d31913d67a05add0f3d6054e88d1 > src/main/java/org/apache/aurora/scheduler/state/StateModule.java > 0e0f90b670bbbcd6cb3aa302ce4a9abfe70ea979 > src/main/java/org/apache/aurora/scheduler/storage/HostMaintenanceStore.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/Storage.java > da5534f886e032ca5a182f3704aa335ff680b258 > > src/main/java/org/apache/aurora/scheduler/storage/durability/DurableStorage.java > f1fdc275d3958a36bbe79110d70dfeba640a948a > src/main/java/org/apache/aurora/scheduler/storage/durability/Loader.java > 10864f122eff5027c88d835baae6de483d960218 > > src/main/java/org/apache/aurora/scheduler/storage/durability/WriteRecorder.java > 8d70cae35289a9e36142bab288cf0c9398ebd2d4 > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotService.java > b30de881eafa3226fdc32383b4e9bfd33ca912a5 > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotterImpl.java > 4b52be02001e704f4b1a5f447226ac8c2386e3fd > > src/main/java/org/apache/aurora/scheduler/storage/mem/MemHostMaintenanceStore.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorage.java > 9f324b010db7e351e98b257d8fc8fecfeac81268 > src/main/java/org/apache/aurora/scheduler/storage/mem/MemStorageModule.java > edcea09b4d206cfddb642074237b031ad71cff13 > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java > e88cad6cf12312512e6840329db7ca7134ceaae6 > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 9fc0416086dd3eb2e2f4e8f659da59fcdea2b22b > src/main/python/apache/aurora/admin/admin_util.py > 8240e8093160623b4c30dd212a88b8e122fd9856 > src/main/python/apache/aurora/admin/host_maintenance.py > 83fc2b6ece40d3436cc7de7a034f95224235fcfd > src/main/python/apache/aurora/admin/maintenance.py > 942a237f47a6e0416bbaf244278685477e0f407d > src/main/python/apache/aurora/client/api/__init__.py > f6fd1dd6d7c2bdd5bca3037f501b36badab78c75 > src/main/python/apache/aurora/client/cli/context.py > 06b194114a7f44a61943e0932973e71b53f239b4 > src/main/python/apache/aurora/client/cli/jobs.py > 536d04a21d32c4e586dc943a6f9b0ad0143354a3 > src/main/python/apache/aurora/config/schema/base.py > a629bcd1261e5959da0a8458a55545d4e2c2a7a5 > src/main/python/apache/aurora/config/thrift.py > 6d2dde6e964daa68bf6f0e5bbbffecc5bd8c0431 > src/main/python/apache/aurora/executor/executor_vars.py > 561f9452aedda4cc695c84a2a850bdd7e1d65dec > src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java > 63c338e5bbdf60de0fba8d68c6613904abb93fa8 > src/test/java/org/apache/aurora/scheduler/base/TaskTestUtil.java > 778148a7c033cba9004954cabc33a2b1d003dccf > src/test/java/org/apache/aurora/scheduler/config/CommandLineTest.java > e66ec116112df164106598d9ff0bc9e8f465e44f > > src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java > 749ffeac6cb851f32bba7606390203d7a046a0e6 > > src/test/java/org/apache/aurora/scheduler/mesos/MesosCallbackHandlerTest.java > c6163bbabc7e7748f167b679893a93f58e4ef1ac > src/test/java/org/apache/aurora/scheduler/sla/SlaManagerTest.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/sla/SlaModuleTest.java > d37e7a07e9258bc8c0758bf50aece5b79025126b > > src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java > 770846e84e9980ea3dbf9e1c46b0d45c5488c5b3 > > src/test/java/org/apache/aurora/scheduler/storage/AbstractHostMaintenanceStoreTest.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/storage/backup/RecoveryTest.java > ba03ff94bb5fee2b09a6660a9ad759cece7449f1 > > src/test/java/org/apache/aurora/scheduler/storage/durability/DataCompatibilityTest.java > 31f9545d83a950064df646ef6ba8a95234cf89ec > > src/test/java/org/apache/aurora/scheduler/storage/durability/DurableStorageTest.java > 3dd9ce4039b223cb6156462d089f7062a1cde772 > > src/test/java/org/apache/aurora/scheduler/storage/durability/WriteRecorderTest.java > 27c8c829cd1e417dd5e60a8e9415331ca4a7c918 > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotterImplIT.java > be07361a27afefa21cc2ba76ce82531a418d9814 > > src/test/java/org/apache/aurora/scheduler/storage/mem/MemHostMaintenanceStoreTest.java > PRE-CREATION > > src/test/java/org/apache/aurora/scheduler/storage/testing/StorageTestUtil.java > d59118be13342da9003b0bcb97e12e477d9edf8f > > src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java > 2cf66d8154ad3795989ee9026e45af1be509f244 > src/test/java/org/apache/aurora/scheduler/thrift/ThriftIT.java > 40851c419e4d62e6545959eebc0ce144fdecc697 > > src/test/java/org/apache/aurora/scheduler/thrift/aop/MockDecoratedThrift.java > d412090c292691305f01bccd1596fb0f6bb003ad > src/test/python/apache/aurora/admin/test_maintenance.py > ca0239b157f9f9053821af0328b9448703386cd4 > src/test/python/apache/aurora/api_util.py > 3fc9b478cc9aada0503e8ed8698a37b4ed926cdd > src/test/python/apache/aurora/client/api/test_scheduler_client.py > f2a2eae1539f7f6dff6855e4122cc41c6cbb0f7b > src/test/python/apache/aurora/client/cli/test_add.py > b22b9f72fbddb553bfc33b1bd8e10636a8d887a6 > src/test/python/apache/aurora/client/cli/test_inspect.py > e4f43d0573c7862adc9bc679f4cea40cc76eac38 > src/test/python/apache/aurora/client/cli/test_kill.py > 0e859dc8618a044b2a4a6f73f45cab4a7ffcce4e > src/test/python/apache/aurora/config/test_thrift.py > 8e1d0e177959af12b97bdd1cd47845b72bc12fe1 > > src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/current/removeHostMaintenanceRequest > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/current/saveCronJob > 88e1c36a1aa2d192b95963f7aa36e243a447e4af > > src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/current/saveHostMaintenanceRequest > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/current/saveJobUpdate > 32fdcdacde58345cdd6c4b449b82c0c90c2b2aae > > src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/current/saveTasks > 4323031ec6bd128576c2a43ebc11f04a9f046e2f > > src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/read-compatible/16-saveHostMaintenanceRequest > PRE-CREATION > > src/test/resources/org/apache/aurora/scheduler/storage/durability/goldens/read-compatible/17-removeHostMaintenanceRequest > PRE-CREATION > src/test/sh/org/apache/aurora/e2e/sla_policy.aurora PRE-CREATION > ui/src/main/js/components/TaskConfigSummary.js > 64880f4bd5c5358287ef481df455f6355fedd7d6 > > > Diff: https://reviews.apache.org/r/66716/diff/8/ > > > Testing > ------- > > ./build-support/jenkins/build.sh > > > Thanks, > > Santhosh Kumar Shanmugham > >
