----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66716/#review203276 -----------------------------------------------------------
I have done a first quick pass. I will have a second closer look once the storage patch has landed. src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java Lines 74 (patched) <https://reviews.apache.org/r/66716/#comment285381> I think we should make this value configurable for operators. src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java Lines 248-259 (patched) <https://reviews.apache.org/r/66716/#comment285385> 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. src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java Lines 276 (patched) <https://reviews.apache.org/r/66716/#comment285383> `"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. src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java Lines 280 (patched) <https://reviews.apache.org/r/66716/#comment285399> 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. src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java Lines 303 (patched) <https://reviews.apache.org/r/66716/#comment285393> 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. src/main/java/org/apache/aurora/scheduler/sla/SlaManager.java Lines 384 (patched) <https://reviews.apache.org/r/66716/#comment285387> isProduction is deprecated. You will need to check the appropriate tier config here. - Stephan Erb On May 15, 2018, 7:16 p.m., Santhosh Kumar Shanmugham wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66716/ > ----------------------------------------------------------- > > (Updated May 15, 2018, 7:16 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 > ----- > > api/src/main/thrift/org/apache/aurora/gen/api.thrift > ef754e32172e7490a47a13e7b526f243ffa3efeb > api/src/main/thrift/org/apache/aurora/gen/storage.thrift > b79e2045ccda05d5058565f81988dfe33feea8f1 > 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/SnapshotDeduplicator.java > 9733ffe74b107f336858657550156ddb1f1dd215 > 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/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_inspect.py > e4f43d0573c7862adc9bc679f4cea40cc76eac38 > 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/7/ > > > Testing > ------- > > ./build-support/jenkins/build.sh > > > Thanks, > > Santhosh Kumar Shanmugham > >
