Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
ava 03a0e8485d1a392f107fda5b4af05b7f8f6067c6 src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 203f62bacc47470545d095e4d25f7e0f25990ed9 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java a177b301203143539b052524d14043ec8a85a46d src/main/java/org/apache/aurora/scheduler/stats/AsyncStatsModule.java 40451e91aed45866c2030d901160cc4e084834df src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java 059fbb86a575f5b3d78a63c9a7b5a9eebb6cb3ae src/main/resources/org/apache/aurora/scheduler/tiers.json 34ddb1dc769a73115c209c9b2ee158cd364392d8 src/test/java/org/apache/aurora/scheduler/TierManagerTest.java 82e40d509d84c37a19b6a9ef942283d908833840 src/test/java/org/apache/aurora/scheduler/app/SchedulerIT.java a7c9c83eebbbea7ae8a6c807f98d3ce8bd050137 src/test/java/org/apache/aurora/scheduler/base/InstanceKeysTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java 50d7499f4332a3feb0e2301cb707f2cea6bb2e98 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java bf9c2b46b186125061c29ac777134a871c727066 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java d2fe7a3d78c9c12ca3d0b40843eb7351217fe1fb src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 30699596a1c95199df7504f62c5c18cab1be1c6c src/test/java/org/apache/aurora/scheduler/mesos/FrameworkInfoFactoryImplTest.java b22f047ba8ead5d840e13e379ec4471288b21118 src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 93cc34cf8393f969087cd0fd6f577228c00170e9 src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java d7addc0effb60c196cf339081ad81de541d05385 src/test/java/org/apache/aurora/scheduler/preemptor/PreemptionVictimFilterTest.java c39b00d65c8c2f2f74c836c6117a651fb7f9cd05 src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/PortMapperTest.java cef795bff131df46a50b632eaf2261df0b8dc3e7 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java 676d305d257585e53f0a05b359ba7eb11f1b23be src/test/java/org/apache/aurora/scheduler/scheduling/ReservationTimeoutCalculatorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/sla/SlaTestUtil.java 78f440f7546de9ed6842cb51db02b3bddc9a74ff src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 016859ca3bf83f64d2576b4c7109729770f9e25c src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 0361b36be3bc1f7d925b616d67dde5c64fd7a909 src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c826a54809ea8bcd25fa04a8fa15ecd414a53e30 src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora b85907a955a1ba83012a26912938b6466f0476ac src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 1a81dc5dcde0400510c5576ac523f4d3f14424ca Diff: https://reviews.apache.org/r/57487/diff/8/ Changes: https://reviews.apache.org/r/57487/diff/7-8/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
d2fe7a3d78c9c12ca3d0b40843eb7351217fe1fb src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 30699596a1c95199df7504f62c5c18cab1be1c6c src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 93cc34cf8393f969087cd0fd6f577228c00170e9 src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java d7addc0effb60c196cf339081ad81de541d05385 src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/PortMapperTest.java cef795bff131df46a50b632eaf2261df0b8dc3e7 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java 676d305d257585e53f0a05b359ba7eb11f1b23be src/test/java/org/apache/aurora/scheduler/scheduling/ReservationTimeoutCalculatorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/sla/SlaTestUtil.java 78f440f7546de9ed6842cb51db02b3bddc9a74ff src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 0361b36be3bc1f7d925b616d67dde5c64fd7a909 src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c826a54809ea8bcd25fa04a8fa15ecd414a53e30 src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora b85907a955a1ba83012a26912938b6466f0476ac src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 1a81dc5dcde0400510c5576ac523f4d3f14424ca Diff: https://reviews.apache.org/r/57487/diff/7/ Changes: https://reviews.apache.org/r/57487/diff/6-7/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
/test/java/org/apache/aurora/scheduler/TierManagerTest.java 82e40d509d84c37a19b6a9ef942283d908833840 src/test/java/org/apache/aurora/scheduler/configuration/ConfigurationManagerTest.java 50d7499f4332a3feb0e2301cb707f2cea6bb2e98 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java d2fe7a3d78c9c12ca3d0b40843eb7351217fe1fb src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 30699596a1c95199df7504f62c5c18cab1be1c6c src/test/java/org/apache/aurora/scheduler/mesos/MesosTaskFactoryImplTest.java 93cc34cf8393f969087cd0fd6f577228c00170e9 src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java d7addc0effb60c196cf339081ad81de541d05385 src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/PortMapperTest.java cef795bff131df46a50b632eaf2261df0b8dc3e7 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java 676d305d257585e53f0a05b359ba7eb11f1b23be src/test/java/org/apache/aurora/scheduler/scheduling/ReservationTimeoutCalculatorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/sla/SlaTestUtil.java 78f440f7546de9ed6842cb51db02b3bddc9a74ff src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 src/test/sh/org/apache/aurora/e2e/http/http_example.aurora 0361b36be3bc1f7d925b616d67dde5c64fd7a909 src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora c826a54809ea8bcd25fa04a8fa15ecd414a53e30 src/test/sh/org/apache/aurora/e2e/http/http_example_updated.aurora b85907a955a1ba83012a26912938b6466f0476ac src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 1a81dc5dcde0400510c5576ac523f4d3f14424ca Diff: https://reviews.apache.org/r/57487/diff/6/ Changes: https://reviews.apache.org/r/57487/diff/5-6/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/scheduling/ReservationTimeoutCalculatorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/sla/SlaTestUtil.java 78f440f7546de9ed6842cb51db02b3bddc9a74ff src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/57487/diff/5/ Changes: https://reviews.apache.org/r/57487/diff/4-5/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
> On March 30, 2017, 11:56 p.m., David McLaughlin wrote: > > The motivation for this is a performance optimization (less Scheduling loop > > overhead + cache locality on the target host). So why should that decision > > be encoded in the service tier? We'd want every single task using this and > > wouldn't want users even knowing about it. And we still want to have the > > preferred vs preemptible distinction. > > > > Currently a task restart is a powerful tool to undo a bad scheduling round > > or for whatever reason to get off a host - e.g. to get away from a noisy > > neighbor or a machine that's close to falling over. If I'm reading this > > patch correctly, they lose this ability after this change? Or at least the > > change is now - kill the task, wait for some operator defined timeout and > > then schedule it again with the original config. > > > > What happens when we want to extend the use of Dynamic Reservations and > > give users control over when they are collected. What tier would we use > > then? How would reserved offers be collected? It seems like this > > implementation is not future proof at all. David, thanks for your comment. I would add the following to performance optimization as improvements and features that this patch will offer: * Consistent MTTA for any size when upgrading, irrespective of cluster capacity and demand, assuming an upgrade does not increase the resource vector (sizing down is OK). * Shorter MTTR for tasks using Docker or unified containerizer, reserved tasks will get consistent placement of each task on the same host, resulting in less work for the Mesos or Docker fetcher as host’s warm cache can be leveraged and previous image layer already exist on each host. * After a job is placed on each host, task failures cannot be in a PENDING state transition as we guarantee resource availability. * This implementation lays foundation for support of persistent volumes in Aurora. The way tier is added, you absolutely can make a reserved job preemptible. All you would do is specify a new tier definition in tiers.json and set both 'reserved' and 'preemptable' to `True`. About restarts, you bring up a good point. I would like to add that if a task does not have a `reserved` set to True inside `TierInfo` then nothing changes and restarts proceed by rescheduling the task onto different hosts. However, if a task will want reserved resources, it implies to us that they want "stickiness" so the task would be scheduled on the same host. I feel like that contradicts use case of trying to get away from noisy neighbors and yes, the story is not great for this case. We can brainstorm on possible solutions for this use case. If this would be an immediately required feature, we can add an `unreserve` operation to any offer that comes back from a reserved task before rescheduling. How does that sound? Would you elaborate what you are referring to about control over dynamically reserved resources? Do we currently give users a control beyond host constraints at the moment? Currently reserved offers are not collected but with @serb's nice suggestion are simply expired if offer is unused. To collect them, we can bring back the `OfferReconciler` if the complexity warrants it. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57487/#review170659 --- On March 30, 2017, 11:20 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57487/ > --- > > (Updated March 30, 2017, 11:20 p.m.) > > > Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. > > > Repository: aurora > > > Description > --- > > Esteemed reviewers, here is the latest iteration on the implementation of > dynamic reservations. Some changes are merging of the patches into a single > one, updated design document with a more high level overview and user stories > told from an operator’s point of view. Unit TESTS are going to be done as > soon as we agree on the approach, as I have tested this patch on local > vagrant and a multi-node dev cluster. Jenkins build is expected to fail as > tested are incomplete. > > For reference, here are previous two patches which feedback I addressed in > this new single patch. > Previous 2 patches: > https://reviews.apache.org/r/56690/ > https://reviews.apache.org/r/56691/ > > RFC document: > https://docs.google.com/document/d/15n2
Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
d7addc0effb60c196cf339081ad81de541d05385 src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/scheduling/ReservationTimeoutCalculatorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/sla/SlaTestUtil.java 78f440f7546de9ed6842cb51db02b3bddc9a74ff src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/57487/diff/4/ Changes: https://reviews.apache.org/r/57487/diff/3-4/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/scheduling/ReservationTimeoutCalculatorTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/sla/SlaTestUtil.java 78f440f7546de9ed6842cb51db02b3bddc9a74ff src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/57487/diff/3/ Changes: https://reviews.apache.org/r/57487/diff/2-3/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/57487/diff/2/ Changes: https://reviews.apache.org/r/57487/diff/1-2/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Re: Review Request 57487: Implementation of Dynamic Reservations Proposal
/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/57487/diff/1/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Review Request 57487: merge
/OfferReconcilerTest.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/scheduling/TaskSchedulerImplTest.java fa1a81785802b82542030e1aae786fe9570d9827 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/57487/diff/1/ Testing --- Tested on local vagrant for following scenarios: Reserving a task Making sure returned offer comes back Making sure offer is unreserved Thanks, Dmitriy Shirchenko
Re: Review Request 56690: [Patch 1/2] RFC for first patch implementing scheduling changes for Dynamic Reservations
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56690/ --- (Updated March 2, 2017, 1:12 p.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1819 https://issues.apache.org/jira/browse/AURORA-1819 Repository: aurora Description (updated) --- This is an RFC (without tests) for dynamic reservations proposal. If there is consensus on the approach, I will add tests. This patch was also tested locally and works as expected. RFC document: https://docs.google.com/document/d/15n29HSQPXuFrnxZAgfVINTRP1Iv47_jfcstJNuMwr5A Design Doc: https://docs.google.com/document/d/1L2EKEcKKBPmuxRviSUebyuqiNwaO-2hsITBjt3SgWvE Diffs - src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java f2296a9d7a88be7e43124370edecfe64415df00f src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/TaskVars.java 676dfd9f9d7ee0633c05424f788fd0ab116976bb src/main/java/org/apache/aurora/scheduler/TierInfo.java c45b949ae7946fc92d7e62f94696ddc4f0790cfa src/main/java/org/apache/aurora/scheduler/TierManager.java c6ad2b1c48673ca2c14ddd308684d81ce536beca src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 src/main/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilter.java f6c759f03c4152ae93317692fc9db202fe251122 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java bb1a960a4c77f48b0ceaa213bd27546551f384f9 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 60097d91d836e2686d6e90571f13a2fbfd88ae14 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 0d639f66db456858278b0485c91c40975c3b45ac src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java e16e36ed360ef9ca371df9084365ea88cfb6e7ce src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java 9aa263a9cfae03a9a0c5bc7fe3a1405397d3009c src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 203f62bacc47470545d095e4d25f7e0f25990ed9 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java da378e84ee65a658ff2382489d3ab6d5f6451b5f src/main/resources/org/apache/aurora/scheduler/tiers.json 34ddb1dc769a73115c209c9b2ee158cd364392d8 src/test/java/org/apache/aurora/scheduler/TierManagerTest.java 82e40d509d84c37a19b6a9ef942283d908833840 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/56690/diff/5/ Testing --- Thanks, Dmitriy Shirchenko
Re: Review Request 56690: [Patch 1/2] RFC for first patch implementing scheduling changes for Dynamic Reservations
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56690/ --- (Updated Feb. 28, 2017, 9:25 p.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Changes --- Fixing checkstyle. Bugs: AURORA-1819 https://issues.apache.org/jira/browse/AURORA-1819 Repository: aurora Description --- This is an RFC (without tests) for dynamic reservations proposal. If there is consensus on the approach, I will add tests. This patch was also tested locally and works as expected. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java f2296a9d7a88be7e43124370edecfe64415df00f src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/TaskVars.java 676dfd9f9d7ee0633c05424f788fd0ab116976bb src/main/java/org/apache/aurora/scheduler/TierInfo.java c45b949ae7946fc92d7e62f94696ddc4f0790cfa src/main/java/org/apache/aurora/scheduler/TierManager.java c6ad2b1c48673ca2c14ddd308684d81ce536beca src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 src/main/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilter.java f6c759f03c4152ae93317692fc9db202fe251122 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java bb1a960a4c77f48b0ceaa213bd27546551f384f9 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 60097d91d836e2686d6e90571f13a2fbfd88ae14 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 0d639f66db456858278b0485c91c40975c3b45ac src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java e16e36ed360ef9ca371df9084365ea88cfb6e7ce src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java 9aa263a9cfae03a9a0c5bc7fe3a1405397d3009c src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 203f62bacc47470545d095e4d25f7e0f25990ed9 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java da378e84ee65a658ff2382489d3ab6d5f6451b5f src/main/resources/org/apache/aurora/scheduler/tiers.json 34ddb1dc769a73115c209c9b2ee158cd364392d8 src/test/java/org/apache/aurora/scheduler/TierManagerTest.java 82e40d509d84c37a19b6a9ef942283d908833840 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/56690/diff/ Testing --- Thanks, Dmitriy Shirchenko
Re: Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/ --- (Updated Feb. 24, 2017, 11:59 p.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1816 https://issues.apache.org/jira/browse/AURORA-1816 Repository: aurora Description (updated) --- Second patch that implements mechanism for unreserving previously reserved resources for dynamic reservations proposal. Impelments OfferReconciler outlined in the design doc [1]. [1] https://docs.google.com/document/d/1L2EKEcKKBPmuxRviSUebyuqiNwaO-2hsITBjt3SgWvE/edit#heading=h.n5rlum8ejc3o Diffs - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/HostOffer.java 23f0600d64e1e15f4856f397e839e3d1c87f3b96 src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 0637eb7f85125cf70b588d56fa7dc88130947837 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 30699596a1c95199df7504f62c5c18cab1be1c6c src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/56691/diff/ Testing (updated) --- Tested locally and a devel cluster. Thanks, Dmitriy Shirchenko
Re: Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/ --- (Updated Feb. 24, 2017, 11:50 p.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1816 https://issues.apache.org/jira/browse/AURORA-1816 Repository: aurora Description --- Second patch that implements mechanism for unreserving previously reserved resources for dynamic reservations proposal. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/HostOffer.java 23f0600d64e1e15f4856f397e839e3d1c87f3b96 src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 0637eb7f85125cf70b588d56fa7dc88130947837 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 30699596a1c95199df7504f62c5c18cab1be1c6c src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/56691/diff/ Testing --- Ran locally on vagrant. Thanks, Dmitriy Shirchenko
Re: Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
> On Feb. 24, 2017, 9:21 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java, line 114 > > <https://reviews.apache.org/r/56691/diff/4/?file=1643847#file1643847line114> > > > > I don't think you need the `components.size() == 4` here. Vanished. Done. > On Feb. 24, 2017, 9:21 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java, line > > 74 > > <https://reviews.apache.org/r/56691/diff/4/?file=1643850#file1643850line74> > > > > Shouldn't we query for tasks in the `ACTIVE_STATES` and not just > > `RUNNING` here? > > > > To me this seems like a task could be in `PENDING` waiting for it's > > reserved offer back and we unreserve it here. You only want to unreserve tasks that are already running. We do not want to unreserve offers if there is a small chance that the task is making it's way through the system. RUNNING is the only safe state. If task is in PENDING then we do not want to unreserve the offer. > On Feb. 24, 2017, 9:21 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java, line > > 86 > > <https://reviews.apache.org/r/56691/diff/4/?file=1643850#file1643850line86> > > > > We prefer `ImmutableList.of()` for lists of one item. Done. > On Feb. 24, 2017, 9:21 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java, line > > 73 > > <https://reviews.apache.org/r/56691/diff/4/?file=1643850#file1643850line73> > > > > Could the `offerAdded` event handler handle the exception instead of > > letting it go up to the `EventBus`? We could log an errror and increment an > > metric. Done. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/#review166759 --- On Feb. 24, 2017, 11:02 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56691/ > --- > > (Updated Feb. 24, 2017, 11:02 p.m.) > > > Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. > > > Bugs: AURORA-1816 > https://issues.apache.org/jira/browse/AURORA-1816 > > > Repository: aurora > > > Description > --- > > Second patch that implements mechanism for unreserving previously reserved > resources for dynamic reservations proposal. > > > Diffs > - > > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java > 6f2ca35c5d83dde29c24865b4826d4932e96da80 > src/main/java/org/apache/aurora/scheduler/HostOffer.java > 23f0600d64e1e15f4856f397e839e3d1c87f3b96 > src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java > b12ac83168401c15fb1d30179ea8e4816f09cd3d > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 0637eb7f85125cf70b588d56fa7dc88130947837 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 > src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java > 202cae96ffc5b49e638b973a273f7983137b5baf > src/test/java/org/apache/aurora/scheduler/http/OffersTest.java > 30699596a1c95199df7504f62c5c18cab1be1c6c > src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > 49d4e82cc03144b80292fe43066a6cc4d7aed88f > src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/56691/diff/ > > > Testing > --- > > Ran locally on vagrant. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 56690: [Patch 1/2] RFC for first patch implementing scheduling changes for Dynamic Reservations
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56690/ --- (Updated Feb. 24, 2017, 10:47 p.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1819 https://issues.apache.org/jira/browse/AURORA-1819 Repository: aurora Description --- This is an RFC (without tests) for dynamic reservations proposal. If there is consensus on the approach, I will add tests. This patch was also tested locally and works as expected. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java f2296a9d7a88be7e43124370edecfe64415df00f src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/TaskVars.java 676dfd9f9d7ee0633c05424f788fd0ab116976bb src/main/java/org/apache/aurora/scheduler/TierInfo.java c45b949ae7946fc92d7e62f94696ddc4f0790cfa src/main/java/org/apache/aurora/scheduler/TierManager.java c6ad2b1c48673ca2c14ddd308684d81ce536beca src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 src/main/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilter.java f6c759f03c4152ae93317692fc9db202fe251122 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java bb1a960a4c77f48b0ceaa213bd27546551f384f9 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 60097d91d836e2686d6e90571f13a2fbfd88ae14 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 0d639f66db456858278b0485c91c40975c3b45ac src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java e16e36ed360ef9ca371df9084365ea88cfb6e7ce src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java 9aa263a9cfae03a9a0c5bc7fe3a1405397d3009c src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 203f62bacc47470545d095e4d25f7e0f25990ed9 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java da378e84ee65a658ff2382489d3ab6d5f6451b5f src/main/resources/org/apache/aurora/scheduler/tiers.json 34ddb1dc769a73115c209c9b2ee158cd364392d8 src/test/java/org/apache/aurora/scheduler/TierManagerTest.java 82e40d509d84c37a19b6a9ef942283d908833840 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/56690/diff/ Testing --- Thanks, Dmitriy Shirchenko
Re: Review Request 56690: [Patch 1/2] RFC for first patch implementing scheduling changes for Dynamic Reservations
rovide enough information > > for someone debugging a live cluster. Done. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java, line 114 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646599#file1646599line114> > > > > Whats the reason for the `components.size() == 4` here? Leftover code. Vanished. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line > > 122 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646604#file1646604line122> > > > > You need to do `this.driverSettings = requireNonNull(driverSettings)` > > here to guard against null pointers. Done. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java, line > > 358 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646604#file1646604line358> > > > > A comment here explaining that we want to label all resources would > > make this bit clearer. > > > > I don't think we do `clearXXX().addAllXXX()` anywhere else in the code. > > > > Also does protobuf not provide a `setXXX` for collections? Yeah, this is the only place where we used to clear resources and readd them. Nothing changes. There is a setResources method but I believe you end up doing the same thing by just looping over the list and placing them into the correct index [1] http://mesos.apache.org/api/latest/java/org/apache/mesos/Protos.ExecutorInfo.Builder.html#setResources(int,%20org.apache.mesos.Protos.Resource.Builder) > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, > > line 201 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646602#file1646602line201> > > > > Could you elaborate on what you mean by "cluster maintenance" here? > > Seems like a copy and paste error from above. Done. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, > > line 315 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646602#file1646602line315> > > > > This needs javadoc. Done. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, > > line 332 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646602#file1646602line332> > > > > This equals is not the same as the other POJOs. Look at > > `UnusedResource` for inspiration. Done. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, > > line 270 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646602#file1646602line270> > > > > `Optional<List>` is a smell to me. > > > > Is it possible to model this just as `List` where the empty list is > > equal to `Optional.absent()`? Done. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 74 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646596#file1646596line74> > > > > I noticed from the code below that the mutator methods only act on > > copies of the teir info. > > > > Have you considered having the mutator methods only return copies? > > > > This means that the impementations of `unReserve` `reReserve` only > > return copies. > > > > This would be good for performance because we no longer need to return > > a copy of tier info for every access, and we only do the copying when > > necessary. Done. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 77 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646596#file1646596line77> > > > > Could we condense `unReserve()` and `reReserve` to just be > > `setReservation(bool b)`? > > > > I think it is more inline with our style elsewhere for mutators.getTier Done. > On Feb. 24, 2017, 9 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/TierInfo.java, line 109 > > <https://reviews.apache.org/r/56690/diff/3/?file=1646596#file1646596line109> > > > > If you accept my suggestion above about making copying implicit in the > > mutators, this would need to become private. Done. >
Re: Review Request 56690: [Patch 1/2] RFC for first patch implementing scheduling changes for Dynamic Reservations
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56690/ --- (Updated Feb. 24, 2017, 12:28 a.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1819 https://issues.apache.org/jira/browse/AURORA-1819 Repository: aurora Description --- This is an RFC (without tests) for dynamic reservations proposal. If there is consensus on the approach, I will add tests. This patch was also tested locally and works as expected. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java f2296a9d7a88be7e43124370edecfe64415df00f src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/TaskVars.java 676dfd9f9d7ee0633c05424f788fd0ab116976bb src/main/java/org/apache/aurora/scheduler/TierInfo.java c45b949ae7946fc92d7e62f94696ddc4f0790cfa src/main/java/org/apache/aurora/scheduler/TierManager.java c6ad2b1c48673ca2c14ddd308684d81ce536beca src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java f0b148cd158d61cd89cc51dca9f3fa4c6feb1b49 src/main/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilter.java f6c759f03c4152ae93317692fc9db202fe251122 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java bb1a960a4c77f48b0ceaa213bd27546551f384f9 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 60097d91d836e2686d6e90571f13a2fbfd88ae14 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 0d639f66db456858278b0485c91c40975c3b45ac src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java e16e36ed360ef9ca371df9084365ea88cfb6e7ce src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java 9aa263a9cfae03a9a0c5bc7fe3a1405397d3009c src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 203f62bacc47470545d095e4d25f7e0f25990ed9 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java da378e84ee65a658ff2382489d3ab6d5f6451b5f src/main/resources/org/apache/aurora/scheduler/tiers.json 34ddb1dc769a73115c209c9b2ee158cd364392d8 src/test/java/org/apache/aurora/scheduler/TierManagerTest.java 82e40d509d84c37a19b6a9ef942283d908833840 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java dded9c34749cf599d197ed312ffb6bf63b6033f1 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java b8b8edb1a21ba89b8b60f8f8451c8c776fc23ae8 src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java e04f6113c43eca4555ee0719f8208d7c4ebb8d61 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java cf2d25ec2e407df7159e0021ddb44adf937e1777 Diff: https://reviews.apache.org/r/56690/diff/ Testing --- Thanks, Dmitriy Shirchenko
Re: Review Request 56690: [Patch 1/2] RFC for first patch implementing scheduling changes for Dynamic Reservations
> On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java, > > lines 99-100 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634087#file1634087line99> > > > > To short-circuit both loops this should probably be a return. Done. > On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/resources/org/apache/aurora/scheduler/tiers.json, lines 29-33 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634095#file1634095line29> > > > > Shoudn't reservations be always backed by quota to prevent abuse? we don't use Aurora's built-in quota management, instead, relying on our own so all jobs regardless of tier do not run as production. i can delete this tier as i think these are for reference. > On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/TierInfo.java, lines 67-69 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634080#file1634080line67> > > > > Copy paste error Done. > On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java, lines > > 51-63 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634082#file1634082line51> > > > > `IAssignedTask` has a `task` which in turn has a jobkey. There should > > therfore be no need to parse this from the Mesos `TaskInfo`. Done. > On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, lines > > 213-231 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634094#file1634094line213> > > > > This reads like as if there is a more general concept of > > "soft-constraints" waiting to be discovered. In other words: We want > > constraints that are lifted once we cannot satisfy them for a certain > > amount of time. See https://issues.apache.org/jira/browse/AURORA-173 for > > details. > > > > I imagine the TaskAssigner could keep its current behavior and only > > assign a task if there is no veto for it. It is the responsibility of an > > out-of-band mechanism to ensure we either don't generate vetos for expired > > soft-constraints, or we have a way to check if a generated veto is already > > expired or not. > > > > (One very rudimentary implementation could probably look similar to > > `TaskTimeout.java` but for the `PENDING` state. However there are most > > likely also other/better ways to do this) Zameer had thoughts on this.^ > On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, > > lines 405-406 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634086#file1634086line405> > > > > Please add a doc string. I may not have completely understood why this > > is absolutely needed. > > > > We should have a very good reason to break the existing interface. Done. > On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 169 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634094#file1634094line169> > > > > Please make sure you always use the built-in formatting of the logger > > methods. Otherwise we will assemble strings in memory, only to be discarded > > by the logger afterwards :-) Done. > On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java, line 211 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634094#file1634094line211> > > > > Same here (and in some other places) Done. Hopefully I caught all of the places. > On Feb. 22, 2017, 11:31 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/TierManager.java, lines 100-105 > > <https://reviews.apache.org/r/56690/diff/2/?file=1634081#file1634081line100> > > > > In addition to my concerns about the mutable tier config, the `copyOf` > > might come with a performance tax. > > > > `getTier` is on a hot code path and I therefore recently rewrote the > > `checkArgument` here to only construct the String if the argument is > > invalid. This yielded a significant perf improvement in our scheduling > > benchmarks. Yeah, I could not think of an alternative and am not very happy with this approach myself. Would love to brainstorm if anyone has better ideas. - Dmitriy --- Thi
Re: Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/#review166450 --- @ReviewBot retry - Dmitriy Shirchenko On Feb. 22, 2017, 10:35 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56691/ > --- > > (Updated Feb. 22, 2017, 10:35 p.m.) > > > Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. > > > Bugs: AURORA-1816 > https://issues.apache.org/jira/browse/AURORA-1816 > > > Repository: aurora > > > Description > --- > > Second patch that implements mechanism for unreserving previously reserved > resources for dynamic reservations proposal. > > > Diffs > - > > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java > 6f2ca35c5d83dde29c24865b4826d4932e96da80 > src/main/java/org/apache/aurora/scheduler/HostOffer.java > 23f0600d64e1e15f4856f397e839e3d1c87f3b96 > src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java > b12ac83168401c15fb1d30179ea8e4816f09cd3d > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java > 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 0637eb7f85125cf70b588d56fa7dc88130947837 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 > src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java > 202cae96ffc5b49e638b973a273f7983137b5baf > src/test/java/org/apache/aurora/scheduler/http/OffersTest.java > 30699596a1c95199df7504f62c5c18cab1be1c6c > src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > 49d4e82cc03144b80292fe43066a6cc4d7aed88f > src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/56691/diff/ > > > Testing > --- > > Ran locally on vagrant. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/#review166434 --- @ReviewBot retry - Dmitriy Shirchenko On Feb. 22, 2017, 10:35 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56691/ > --- > > (Updated Feb. 22, 2017, 10:35 p.m.) > > > Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. > > > Bugs: AURORA-1816 > https://issues.apache.org/jira/browse/AURORA-1816 > > > Repository: aurora > > > Description > --- > > Second patch that implements mechanism for unreserving previously reserved > resources for dynamic reservations proposal. > > > Diffs > - > > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java > 6f2ca35c5d83dde29c24865b4826d4932e96da80 > src/main/java/org/apache/aurora/scheduler/HostOffer.java > 23f0600d64e1e15f4856f397e839e3d1c87f3b96 > src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java > b12ac83168401c15fb1d30179ea8e4816f09cd3d > src/main/java/org/apache/aurora/scheduler/base/JobKeys.java > 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 0637eb7f85125cf70b588d56fa7dc88130947837 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 > src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java > 202cae96ffc5b49e638b973a273f7983137b5baf > src/test/java/org/apache/aurora/scheduler/http/OffersTest.java > 30699596a1c95199df7504f62c5c18cab1be1c6c > src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > 49d4e82cc03144b80292fe43066a6cc4d7aed88f > src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/56691/diff/ > > > Testing > --- > > Ran locally on vagrant. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/ --- (Updated Feb. 16, 2017, 12:01 a.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1816 https://issues.apache.org/jira/browse/AURORA-1816 Repository: aurora Description --- Second patch that implements mechanism for unreserving previously reserved resources for dynamic reservations proposal. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/HostOffer.java 23f0600d64e1e15f4856f397e839e3d1c87f3b96 src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 src/main/java/org/apache/aurora/scheduler/base/Query.java c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 0637eb7f85125cf70b588d56fa7dc88130947837 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 30699596a1c95199df7504f62c5c18cab1be1c6c src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/56691/diff/ Testing --- Ran locally on vagrant. Thanks, Dmitriy Shirchenko
Re: Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/ --- (Updated Feb. 15, 2017, 9:09 a.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1816 https://issues.apache.org/jira/browse/AURORA-1816 Repository: aurora Description --- Second patch that implements mechanism for unreserving previously reserved resources for dynamic reservations proposal. Diffs - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/HostOffer.java 23f0600d64e1e15f4856f397e839e3d1c87f3b96 src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 src/main/java/org/apache/aurora/scheduler/base/Query.java c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 0637eb7f85125cf70b588d56fa7dc88130947837 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 30699596a1c95199df7504f62c5c18cab1be1c6c src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/56691/diff/ Testing --- Ran locally on vagrant. Thanks, Dmitriy Shirchenko
Re: Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/ --- (Updated Feb. 15, 2017, 1:48 a.m.) Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1816 https://issues.apache.org/jira/browse/AURORA-1816 Repository: aurora Description --- Second patch that implements mechanism for unreserving previously reserved resources for dynamic reservations proposal. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java 6f2ca35c5d83dde29c24865b4826d4932e96da80 src/main/java/org/apache/aurora/scheduler/HostOffer.java 23f0600d64e1e15f4856f397e839e3d1c87f3b96 src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 src/main/java/org/apache/aurora/scheduler/base/Query.java c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 0637eb7f85125cf70b588d56fa7dc88130947837 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 8c000cb0626bd34f6f30e23fe2b3a045f2b44e35 src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/test/java/org/apache/aurora/scheduler/http/OffersTest.java 30699596a1c95199df7504f62c5c18cab1be1c6c src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 49d4e82cc03144b80292fe43066a6cc4d7aed88f src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/56691/diff/ Testing --- Ran locally on vagrant. Thanks, Dmitriy Shirchenko
Review Request 56691: [Patch 2/2] RFC for 2nd patch implements OfferReconciler for dynamic reservations proposal
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56691/ --- Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1816 https://issues.apache.org/jira/browse/AURORA-1816 Repository: aurora Description --- Second patch that implements mechanism for unreserving previously reserved resources for dynamic reservations proposal. Diffs - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java fbd24ea4a58e28c14a343170de137c0e0ae437a2 src/main/java/org/apache/aurora/scheduler/HostOffer.java ad30bf978ae5aa278fa9b5e01294c43892b08762 src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 src/main/java/org/apache/aurora/scheduler/base/Query.java c76b365f43eb6a3b9b0b63a879b43eb04dcd8fac src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 70b5470b9dad1af838b5222cae5ac86487e2f2e4 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 6c2b6d20658a7fe75725487c9a983e884d9ddfe5 src/main/java/org/apache/aurora/scheduler/offers/OfferReconciler.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/test/java/org/apache/aurora/scheduler/http/OffersTest.java add0eb8f861a36551ed87ba13a75b82c5cd5bdfa src/test/java/org/apache/aurora/scheduler/offers/HostOffers.java PRE-CREATION src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java 5e570b6341c55be8ef27469077932d1ea8378b55 src/test/java/org/apache/aurora/scheduler/offers/OfferReconcilerTest.java PRE-CREATION Diff: https://reviews.apache.org/r/56691/diff/ Testing --- Ran locally on vagrant. Thanks, Dmitriy Shirchenko
Review Request 56690: [Patch 1/2] RFC for first patch implementing scheduling changes for Dynamic Reservations
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56690/ --- Review request for Aurora, Mehrdad Nurolahzade, Stephan Erb, and Zameer Manji. Bugs: AURORA-1819 https://issues.apache.org/jira/browse/AURORA-1819 Repository: aurora Description --- This is an RFC (without tests) for dynamic reservations proposal. If there is consensus on the approach, I will add tests. This patch was also tested locally and works as expected. Diffs - src/jmh/java/org/apache/aurora/benchmark/SchedulingBenchmarks.java f2296a9d7a88be7e43124370edecfe64415df00f src/jmh/java/org/apache/aurora/benchmark/fakes/FakeOfferManager.java fbd24ea4a58e28c14a343170de137c0e0ae437a2 src/main/java/org/apache/aurora/scheduler/TaskVars.java 676dfd9f9d7ee0633c05424f788fd0ab116976bb src/main/java/org/apache/aurora/scheduler/TierInfo.java c45b949ae7946fc92d7e62f94696ddc4f0790cfa src/main/java/org/apache/aurora/scheduler/TierManager.java c6ad2b1c48673ca2c14ddd308684d81ce536beca src/main/java/org/apache/aurora/scheduler/base/InstanceKeys.java b12ac83168401c15fb1d30179ea8e4816f09cd3d src/main/java/org/apache/aurora/scheduler/base/JobKeys.java 0136afb8f6049a6d88cd42b5e3f17d61fcd629d5 src/main/java/org/apache/aurora/scheduler/base/TaskTestUtil.java aba73005019c13ac943be9c53ac58c8ce5bfba94 src/main/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilter.java f6c759f03c4152ae93317692fc9db202fe251122 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java bb1a960a4c77f48b0ceaa213bd27546551f384f9 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 60097d91d836e2686d6e90571f13a2fbfd88ae14 src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java 3d5c3bd139206e970811aa95bd74b78987bb9cfe src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 2b12696d0218a48d71e89df4d3a38c6bf84c74c7 src/main/java/org/apache/aurora/scheduler/offers/OfferSettings.java e16e36ed360ef9ca371df9084365ea88cfb6e7ce src/main/java/org/apache/aurora/scheduler/offers/OffersModule.java 202cae96ffc5b49e638b973a273f7983137b5baf src/main/java/org/apache/aurora/scheduler/resources/ResourceManager.java d751d017522c682a1373932e286ee7b4c447e2aa src/main/java/org/apache/aurora/scheduler/scheduling/TaskScheduler.java 203f62bacc47470545d095e4d25f7e0f25990ed9 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java a030cebea13d92766584643ddda33c50dec7bdbf src/main/resources/org/apache/aurora/scheduler/tiers.json 34ddb1dc769a73115c209c9b2ee158cd364392d8 src/test/java/org/apache/aurora/scheduler/TierManagerTest.java 82e40d509d84c37a19b6a9ef942283d908833840 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java fb8bd85be93280a9b8366b1398a1fa0a5817bb55 src/test/java/org/apache/aurora/scheduler/resources/AcceptedOfferTest.java 2777d7276d55cc0fe75e7470d0dc8182796d67f4 src/test/java/org/apache/aurora/scheduler/resources/ResourceManagerTest.java 133cd23427485ea329ef10489e5d4a0029cb54cd src/test/java/org/apache/aurora/scheduler/resources/ResourceTestUtil.java 7dc8c179736fab93ca03e174b16e104ba0118127 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java f8bb25b612ae42748205901dd4b8cfcdedf5b136 Diff: https://reviews.apache.org/r/56690/diff/ Testing --- Thanks, Dmitriy Shirchenko
Re: Review Request 55902: Capture health check output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55902/#review163004 --- @ReviewBot retry - Dmitriy Shirchenko On Jan. 25, 2017, 6:47 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55902/ > --- > > (Updated Jan. 25, 2017, 6:47 p.m.) > > > Review request for Aurora, Joshua Cohen and Zameer Manji. > > > Bugs: AURORA-1881 > https://issues.apache.org/jira/browse/AURORA-1881 > > > Repository: aurora > > > Description > --- > > Users really could really benefit from seeing the output of the shell health > check failure, so plumbing through the output. > > > Diffs > - > > src/main/python/apache/aurora/common/health_check/shell.py > 58470a48a7a14092eeb8837aada9e358c6922c93 > src/test/python/apache/aurora/common/health_check/test_shell.py > 792ef40028cda112db5b93c4cc37e305937bc351 > > Diff: https://reviews.apache.org/r/55902/diff/ > > > Testing > --- > > added unit tests > e2e tests > screenshot attached. > > > File Attachments > > > Updated screenshot > > https://reviews.apache.org/media/uploaded/files/2017/01/25/90d6ff4f-84f9-4b4d-9b3d-56dadf7027ae__Screen_Shot_2017-01-24_at_5.32.08_PM.png > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 55902: Capture health check output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55902/ --- (Updated Jan. 25, 2017, 6:47 p.m.) Review request for Aurora, Joshua Cohen and Zameer Manji. Bugs: AURORA-1881 https://issues.apache.org/jira/browse/AURORA-1881 Repository: aurora Description --- Users really could really benefit from seeing the output of the shell health check failure, so plumbing through the output. Diffs (updated) - src/main/python/apache/aurora/common/health_check/shell.py 58470a48a7a14092eeb8837aada9e358c6922c93 src/test/python/apache/aurora/common/health_check/test_shell.py 792ef40028cda112db5b93c4cc37e305937bc351 Diff: https://reviews.apache.org/r/55902/diff/ Testing --- added unit tests e2e tests screenshot attached. File Attachments Updated screenshot https://reviews.apache.org/media/uploaded/files/2017/01/25/90d6ff4f-84f9-4b4d-9b3d-56dadf7027ae__Screen_Shot_2017-01-24_at_5.32.08_PM.png Thanks, Dmitriy Shirchenko
Re: Review Request 55902: Capture health check output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55902/ --- (Updated Jan. 25, 2017, 6:37 p.m.) Review request for Aurora, Joshua Cohen and Zameer Manji. Bugs: AURORA-1881 https://issues.apache.org/jira/browse/AURORA-1881 Repository: aurora Description --- Users really could really benefit from seeing the output of the shell health check failure, so plumbing through the output. Diffs (updated) - src/main/python/apache/aurora/common/health_check/shell.py 58470a48a7a14092eeb8837aada9e358c6922c93 src/test/python/apache/aurora/common/health_check/test_shell.py 792ef40028cda112db5b93c4cc37e305937bc351 Diff: https://reviews.apache.org/r/55902/diff/ Testing --- added unit tests e2e tests screenshot attached. File Attachments Updated screenshot https://reviews.apache.org/media/uploaded/files/2017/01/25/90d6ff4f-84f9-4b4d-9b3d-56dadf7027ae__Screen_Shot_2017-01-24_at_5.32.08_PM.png Thanks, Dmitriy Shirchenko
Re: Review Request 55902: Capture health check output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55902/#review162894 --- @ReviewBot retry - Dmitriy Shirchenko On Jan. 25, 2017, 1:34 a.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55902/ > --- > > (Updated Jan. 25, 2017, 1:34 a.m.) > > > Review request for Aurora, Joshua Cohen and Zameer Manji. > > > Bugs: AURORA-1881 > https://issues.apache.org/jira/browse/AURORA-1881 > > > Repository: aurora > > > Description > --- > > Users really could really benefit from seeing the output of the shell health > check failure, so plumbing through the output. > > > Diffs > - > > src/main/python/apache/aurora/common/health_check/shell.py > 58470a48a7a14092eeb8837aada9e358c6922c93 > src/test/python/apache/aurora/common/health_check/test_shell.py > 792ef40028cda112db5b93c4cc37e305937bc351 > > Diff: https://reviews.apache.org/r/55902/diff/ > > > Testing > --- > > added unit tests > e2e tests > screenshot attached. > > > File Attachments > > > Updated screenshot > > https://reviews.apache.org/media/uploaded/files/2017/01/25/90d6ff4f-84f9-4b4d-9b3d-56dadf7027ae__Screen_Shot_2017-01-24_at_5.32.08_PM.png > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 55902: Capture health check output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55902/ --- (Updated Jan. 25, 2017, 1:34 a.m.) Review request for Aurora, Joshua Cohen and Zameer Manji. Changes --- Updating screenshot. Bugs: AURORA-1881 https://issues.apache.org/jira/browse/AURORA-1881 Repository: aurora Description --- Users really could really benefit from seeing the output of the shell health check failure, so plumbing through the output. Diffs - src/main/python/apache/aurora/common/health_check/shell.py 58470a48a7a14092eeb8837aada9e358c6922c93 src/test/python/apache/aurora/common/health_check/test_shell.py 792ef40028cda112db5b93c4cc37e305937bc351 Diff: https://reviews.apache.org/r/55902/diff/ Testing --- added unit tests e2e tests screenshot attached. File Attachments (updated) Updated screenshot https://reviews.apache.org/media/uploaded/files/2017/01/25/90d6ff4f-84f9-4b4d-9b3d-56dadf7027ae__Screen_Shot_2017-01-24_at_5.32.08_PM.png Thanks, Dmitriy Shirchenko
Re: Review Request 55902: Capture health check output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55902/ --- (Updated Jan. 25, 2017, 1:34 a.m.) Review request for Aurora, Joshua Cohen and Zameer Manji. Bugs: AURORA-1881 https://issues.apache.org/jira/browse/AURORA-1881 Repository: aurora Description --- Users really could really benefit from seeing the output of the shell health check failure, so plumbing through the output. Diffs - src/main/python/apache/aurora/common/health_check/shell.py 58470a48a7a14092eeb8837aada9e358c6922c93 src/test/python/apache/aurora/common/health_check/test_shell.py 792ef40028cda112db5b93c4cc37e305937bc351 Diff: https://reviews.apache.org/r/55902/diff/ Testing --- added unit tests e2e tests screenshot attached. File Attachments (updated) Screenshot https://reviews.apache.org/media/uploaded/files/2017/01/25/c4e69424-71ad-4d71-b1f4-895bc6a7821e__Screen_Shot_2017-01-24_at_5.03.06_PM.png Updated screenshot https://reviews.apache.org/media/uploaded/files/2017/01/25/90d6ff4f-84f9-4b4d-9b3d-56dadf7027ae__Screen_Shot_2017-01-24_at_5.32.08_PM.png Thanks, Dmitriy Shirchenko
Review Request 55902: Capture health check output
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55902/ --- Review request for Aurora, Joshua Cohen and Zameer Manji. Bugs: AURORA-1881 https://issues.apache.org/jira/browse/AURORA-1881 Repository: aurora Description --- Users really could really benefit from seeing the output of the shell health check failure, so plumbing through the output. Diffs - src/main/python/apache/aurora/common/health_check/shell.py 58470a48a7a14092eeb8837aada9e358c6922c93 src/test/python/apache/aurora/common/health_check/test_shell.py 792ef40028cda112db5b93c4cc37e305937bc351 Diff: https://reviews.apache.org/r/55902/diff/ Testing --- added unit tests e2e tests screenshot attached. File Attachments Screenshot https://reviews.apache.org/media/uploaded/files/2017/01/25/c4e69424-71ad-4d71-b1f4-895bc6a7821e__Screen_Shot_2017-01-24_at_5.03.06_PM.png Thanks, Dmitriy Shirchenko
Re: Review Request 55105: AURORA-1870 Add finer grained timings to the Snapshot process
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55105/#review160304 --- src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java (line 522) <https://reviews.apache.org/r/55105/#comment231425> for my own knowledge: does Java have a similar concept as Python of decorators, so that you can just wrap functions you want timed with a method that will add those metrics? Sounds super useful to be used in the rest of the code base. - Dmitriy Shirchenko On Dec. 30, 2016, 9:18 p.m., Mehrdad Nurolahzade wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55105/ > --- > > (Updated Dec. 30, 2016, 9:18 p.m.) > > > Review request for Aurora, David McLaughlin and Joshua Cohen. > > > Bugs: AURORA-1870 > https://issues.apache.org/jira/browse/AURORA-1870 > > > Repository: aurora > > > Description > --- > > AURORA-1870 Add finer grained timings to the Snapshot process > > I gave up on `@Timed` interceptor approach because major refactoring is > required in order to have snapshot fields instantiated by Guice through > `Provider` or `@Provides` interfaces. The abstract class approach is much > easier/cleaner. > > > Diffs > - > > > src/main/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImpl.java > 7aa111ec14696ae40f518c42f3c7f45d8ab0e94c > > src/test/java/org/apache/aurora/scheduler/storage/log/SnapshotStoreImplIT.java > f56a1624c7188da175ad3e6de323c3442802f2ea > > Diff: https://reviews.apache.org/r/55105/diff/ > > > Testing > --- > > ``` > $ curl localhost:8081/vars | grep snapshot_restore > % Total% Received % Xferd Average Speed TimeTime Time > Current > Dload Upload Total SpentLeft Speed > 0 00 00 0 0 0 --:--:-- --:--:-- --:--:-- 0 > snapshot_restore_crons_events 1 > snapshot_restore_crons_events_per_sec 0.0 > snapshot_restore_crons_nanos_per_event 0.0 > snapshot_restore_crons_nanos_total 73648 > snapshot_restore_crons_nanos_total_per_sec 0.0 > snapshot_restore_dbscript_events 1 > snapshot_restore_dbscript_events_per_sec 0.0 > snapshot_restore_dbscript_nanos_per_event 0.0 > snapshot_restore_dbscript_nanos_total 1148842021 > snapshot_restore_dbscript_nanos_total_per_sec 0.0 > snapshot_restore_hosts_events 1 > snapshot_restore_hosts_events_per_sec 0.0 > snapshot_restore_hosts_nanos_per_event 0.0 > snapshot_restore_hosts_nanos_total 76166 > snapshot_restore_hosts_nanos_total_per_sec 0.0 > snapshot_restore_job_updates_events 1 > snapshot_restore_job_updates_events_per_sec 0.0 > snapshot_restore_job_updates_nanos_per_event 0.0 > snapshot_restore_job_updates_nanos_total 49482 > snapshot_restore_job_updates_nanos_total_per_sec 0.0 > snapshot_restore_locks_events 1 > snapshot_restore_locks_events_per_sec 0.0 > snapshot_restore_locks_nanos_per_event 0.0 > snapshot_restore_locks_nanos_total 125084 > snapshot_restore_locks_nanos_total_per_sec 0.0 > snapshot_restore_quota_events 1 > snapshot_restore_quota_events_per_sec 0.0 > snapshot_restore_quota_nanos_per_event 0.0 > snapshot_restore_quota_nanos_total 52305 > snapshot_restore_quota_nanos_total_per_sec 0.0 > snapshot_restore_scheduler_metadata_events 1 > snapshot_restore_scheduler_metadata_events_per_sec 0.0 > snapshot_restore_scheduler_metadata_nanos_per_event 0.0 > snapshot_restore_scheduler_metadata_nanos_total 70816 > snapshot_restore_scheduler_metadata_nanos_total_per_sec 0.0 > snapshot_restore_tasks_events 1 > snapshot_restore_tasks_events_per_sec 0.0 > snapshot_restore_tasks_nanos_per_event 0.0 > snapshot_restore_tasks_nanos_total 91377 > snapshot_restore_tasks_nanos_total_per_sec 0.0 > ``` > > ``` > $ aurora_admin scheduler_snapshot devcluster > INFO] Response from scheduler: OK (message: ) > > $ curl localhost:8081/vars | grep snapshot_save > % Total% Received % Xferd Average Speed TimeTime Time > Current > Dload Upload Total SpentLeft Speed > 100 482260 482260 0 3266k 0 --:--:-- --:--:-- --:--:-- 3363k > snapshot_save_crons_events 1 > snapshot_save_crons_events_per_sec 0.0 > snapshot_save_crons_nanos_per_event 0.0 > snapshot_save_crons_nanos_total 466181 > snapshot_save_crons_nanos_total_per_sec 0.0 > snapshot_save_dbscript_events 1 > s
Re: Review Request 52437: Adding support for Ubuntu Xenial packages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52437/#review154818 --- Ship it! Ship It! - Dmitriy Shirchenko On Oct. 12, 2016, 11:17 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52437/ > --- > > (Updated Oct. 12, 2016, 11:17 p.m.) > > > Review request for Aurora, Dmitriy Shirchenko and Zameer Manji. > > > Repository: aurora-packaging > > > Description > --- > > Added builder and test environment for Xenial as well as updated instructions > on how to test it. > > Added distributtion to release-candidate script. > > > Diffs > - > > build-support/release/release-candidate > c08c88529ec036989032869198da7a21dcf6ac35 > builder/deb/ubuntu-xenial/Dockerfile PRE-CREATION > builder/deb/ubuntu-xenial/build.sh PRE-CREATION > specs/debian/aurora-scheduler.startup.sh PRE-CREATION > specs/ubuntu-xenial/aurora-doc.docs PRE-CREATION > specs/ubuntu-xenial/aurora-doc.examples PRE-CREATION > specs/ubuntu-xenial/aurora-pants.ini PRE-CREATION > specs/ubuntu-xenial/aurora-scheduler.default PRE-CREATION > specs/ubuntu-xenial/aurora-scheduler.install PRE-CREATION > specs/ubuntu-xenial/aurora-scheduler.links PRE-CREATION > specs/ubuntu-xenial/aurora-scheduler.postinst PRE-CREATION > specs/ubuntu-xenial/aurora-scheduler.service PRE-CREATION > specs/ubuntu-xenial/aurora-scheduler.startup.sh PRE-CREATION > specs/ubuntu-xenial/aurora-tools.install PRE-CREATION > specs/ubuntu-xenial/aurora-tools.links PRE-CREATION > specs/ubuntu-xenial/changelog PRE-CREATION > specs/ubuntu-xenial/clusters.json PRE-CREATION > specs/ubuntu-xenial/compat PRE-CREATION > specs/ubuntu-xenial/control PRE-CREATION > specs/ubuntu-xenial/copyright PRE-CREATION > specs/ubuntu-xenial/rules PRE-CREATION > specs/ubuntu-xenial/source/format PRE-CREATION > specs/ubuntu-xenial/thermos.default PRE-CREATION > specs/ubuntu-xenial/thermos.dirs PRE-CREATION > specs/ubuntu-xenial/thermos.install PRE-CREATION > specs/ubuntu-xenial/thermos.links PRE-CREATION > specs/ubuntu-xenial/thermos.service PRE-CREATION > test/deb/ubuntu-xenial/README.md PRE-CREATION > test/deb/ubuntu-xenial/Vagrantfile PRE-CREATION > test/deb/ubuntu-xenial/provision.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/52437/diff/ > > > Testing > --- > > Created artifacts using the build-artifacts script. > > Brought a vagrant image up, installed all deb files created by the artifacts > script, started both aurora-scheduler and thermos services, and launched a > sample job. > > > Thanks, > > Renan DelValle > >
Re: Review Request 52437: Adding support for Ubuntu Xenial packages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52437/#review151091 --- Ship it! Ship It! - Dmitriy Shirchenko On Sept. 30, 2016, 7:23 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52437/ > --- > > (Updated Sept. 30, 2016, 7:23 p.m.) > > > Review request for Aurora, Dmitriy Shirchenko and Zameer Manji. > > > Repository: aurora-packaging > > > Description > --- > > Added builder and test environment for Xenial as well as updated instructions > on how to test it. > > Added distributtion to release-candidate script. > > > Diffs > - > > build-support/release/release-candidate > c08c88529ec036989032869198da7a21dcf6ac35 > builder/deb/ubuntu-xenial/Dockerfile PRE-CREATION > builder/deb/ubuntu-xenial/build.sh PRE-CREATION > test/deb/ubuntu-xenial/README.md PRE-CREATION > test/deb/ubuntu-xenial/Vagrantfile PRE-CREATION > test/deb/ubuntu-xenial/provision.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/52437/diff/ > > > Testing > --- > > Created artifacts using the build-artifacts script. > > Brought a vagrant image up, installed all deb files created by the artifacts > script, started both aurora-scheduler and thermos services, and launched a > sample job. > > > Thanks, > > Renan DelValle > >
Re: Review Request 52437: Adding support for Ubuntu Xenial packages
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52437/#review151066 --- builder/deb/ubuntu-xenial/Dockerfile (lines 36 - 40) <https://reviews.apache.org/r/52437/#comment219178> For my own curiousity: is the step of adding gpg keys strictly necessary? Why does ubuntu need it but not debian? - Dmitriy Shirchenko On Sept. 30, 2016, 7:23 p.m., Renan DelValle wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52437/ > --- > > (Updated Sept. 30, 2016, 7:23 p.m.) > > > Review request for Aurora, Dmitriy Shirchenko and Zameer Manji. > > > Repository: aurora-packaging > > > Description > --- > > Added builder and test environment for Xenial as well as updated instructions > on how to test it. > > Added distributtion to release-candidate script. > > > Diffs > - > > build-support/release/release-candidate > c08c88529ec036989032869198da7a21dcf6ac35 > builder/deb/ubuntu-xenial/Dockerfile PRE-CREATION > builder/deb/ubuntu-xenial/build.sh PRE-CREATION > test/deb/ubuntu-xenial/README.md PRE-CREATION > test/deb/ubuntu-xenial/Vagrantfile PRE-CREATION > test/deb/ubuntu-xenial/provision.sh PRE-CREATION > > Diff: https://reviews.apache.org/r/52437/diff/ > > > Testing > --- > > Created artifacts using the build-artifacts script. > > Brought a vagrant image up, installed all deb files created by the artifacts > script, started both aurora-scheduler and thermos services, and launched a > sample job. > > > Thanks, > > Renan DelValle > >
Re: Review Request 52276: Fixing connection leak in webhook by making sure stream is closed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52276/ --- (Updated Sept. 26, 2016, 9:16 p.m.) Review request for Aurora and Zameer Manji. Bugs: AURORA-1783 https://issues.apache.org/jira/browse/AURORA-1783 Repository: aurora Description --- Last refactoring of Webhook did not correctly close out connections so some webhook requests would not complete. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/events/Webhook.java 66c134488f159fc25cfb2c1755b8fa23fbbdb613 src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 71aae983e9c2c34e106a931e4d25691977684b11 src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 6f37baaefb2910e688d7df90f3e94fc282cfb6b6 Diff: https://reviews.apache.org/r/52276/diff/ Testing --- Verified in vagrant + added unit tests. Thanks, Dmitriy Shirchenko
Review Request 52276: Fixing connection leak in webhook by making sure stream is closed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52276/ --- Review request for Aurora and Zameer Manji. Bugs: AURORA-1783 https://issues.apache.org/jira/browse/AURORA-1783 Repository: aurora Description --- Last refactoring of Webhook did not correctly close out connections so some webhook requests would not complete. Diffs - src/main/java/org/apache/aurora/scheduler/events/Webhook.java 66c134488f159fc25cfb2c1755b8fa23fbbdb613 src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java 71aae983e9c2c34e106a931e4d25691977684b11 src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 6f37baaefb2910e688d7df90f3e94fc282cfb6b6 Diff: https://reviews.apache.org/r/52276/diff/ Testing --- Verified in vagrant + added unit tests. Thanks, Dmitriy Shirchenko
Re: Review Request 52094: Add min_consecutive_health_checks in HealthCheckConfig
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52094/#review150118 --- src/main/python/apache/aurora/client/config.py (line 112) <https://reviews.apache.org/r/52094/#comment217976> why was this configuration option deleted? it is still referenced: https://github.com/apache/aurora/blob/master/docs/reference/configuration.md#healthcheckconfig-objects if you are introducing a new option it should be documented. src/main/python/apache/aurora/config/schema/base.py (line 32) <https://reviews.apache.org/r/52094/#comment217977> new default should changed in configuration.md src/main/python/apache/aurora/config/schema/base.py (line 63) <https://reviews.apache.org/r/52094/#comment217978> also needs to be documented. - Dmitriy Shirchenko On Sept. 20, 2016, 7:43 p.m., Kai Huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52094/ > --- > > (Updated Sept. 20, 2016, 7:43 p.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Zameer Manji. > > > Bugs: AURORA-1224 > https://issues.apache.org/jira/browse/AURORA-1224 > > > Repository: aurora > > > Description > --- > > Add min_consecutive_health_checks to HealthCheckConfig. > > [Summary] > HealthCheckConfig should accept a new configuration value that will tell how > many positive consecutive health checks an instance requires to move from > STARTING to RUNNING. > > [Background] > This review depends on the executor change(AURORA-1225). Please see > https://reviews.apache.org/r/51876/ for more details and background. > > [Change List] > 1. Add a configuration value "min_consecutive_health_checks"(default=1) to > HealthCheckConfig struct. > 2. Modify the default value of watch_secs to be 0. > 3. Add a client-side constraint: > initial_interval_secs >= min_consecutive_health_checks * interval_secs > 4. Update the unit tests for health check config in client/config.py, skip > unit tests related to watch_secs. > > > Diffs > - > > src/main/python/apache/aurora/client/config.py > 0186af52f0d7d7e3981ec59bf6a01aafee2bcfb1 > src/main/python/apache/aurora/config/schema/base.py > 845163043b0b7b2f9e7aca14677ca9f094658551 > src/test/python/apache/aurora/client/test_config.py > 5cf68a5145ddf9478baa30453c0bcb73136fa7eb > > Diff: https://reviews.apache.org/r/52094/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > > ./pants test.pytest src/test/python/apache/aurora/client:: > > ./pants test.pytest src/test/python/apache/aurora/config:: > > > Thanks, > > Kai Huang > >
Re: Review Request 52074: switching from launchTask to acceptOffers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/ --- (Updated Sept. 21, 2016, 6:51 p.m.) Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer Manji. Bugs: AURORA-1776 https://issues.apache.org/jira/browse/AURORA-1776 Repository: aurora Description --- Dynamic reservation requires us to use acceptOffers so this is in preparation of that work. acceptOffers is the call to dynamically reserve resources. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 14481468926b82a9bad3ee78c85f176e8ba893e2 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 41b9aab55aa439b13eba3545703ec97b44690444 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 925c025d21bf1d44e0c1d319f6653ecfa8899481 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java a739bce1226d9435fa7d0b18e411064a4e78e49e Diff: https://reviews.apache.org/r/52074/diff/ Testing --- src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Dmitriy Shirchenko
Re: Review Request 52074: switching from launchTask to acceptOffers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/ --- (Updated Sept. 21, 2016, 6:23 p.m.) Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer Manji. Bugs: AURORA-1776 https://issues.apache.org/jira/browse/AURORA-1776 Repository: aurora Description --- Dynamic reservation requires us to use acceptOffers so this is in preparation of that work. acceptOffers is the call to dynamically reserve resources. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 14481468926b82a9bad3ee78c85f176e8ba893e2 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 41b9aab55aa439b13eba3545703ec97b44690444 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 925c025d21bf1d44e0c1d319f6653ecfa8899481 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java a739bce1226d9435fa7d0b18e411064a4e78e49e Diff: https://reviews.apache.org/r/52074/diff/ Testing --- src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Dmitriy Shirchenko
Re: Review Request 52074: switching from launchTask to acceptOffers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/ --- (Updated Sept. 21, 2016, 6:19 p.m.) Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer Manji. Bugs: AURORA-1776 https://issues.apache.org/jira/browse/AURORA-1776 Repository: aurora Description --- Dynamic reservation requires us to use acceptOffers so this is in preparation of that work. acceptOffers is the call to dynamically reserve resources. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 14481468926b82a9bad3ee78c85f176e8ba893e2 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 41b9aab55aa439b13eba3545703ec97b44690444 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 925c025d21bf1d44e0c1d319f6653ecfa8899481 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java a739bce1226d9435fa7d0b18e411064a4e78e49e Diff: https://reviews.apache.org/r/52074/diff/ Testing --- src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Dmitriy Shirchenko
Re: Review Request 52074: switching from launchTask to acceptOffers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/#review149869 --- src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java (lines 50 - 54) <https://reviews.apache.org/r/52074/#comment217606> Agreed. I have been told that Aurora's philosophy is convention first, so I'm following what has been set. - Dmitriy Shirchenko On Sept. 20, 2016, 7:14 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52074/ > --- > > (Updated Sept. 20, 2016, 7:14 p.m.) > > > Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer > Manji. > > > Bugs: AURORA-1776 > https://issues.apache.org/jira/browse/AURORA-1776 > > > Repository: aurora > > > Description > --- > > Dynamic reservation requires us to use acceptOffers so this is in preparation > of that work. acceptOffers is the call to dynamically reserve resources. > > > Diffs > - > > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java > 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a > src/main/java/org/apache/aurora/scheduler/mesos/Driver.java > 14481468926b82a9bad3ee78c85f176e8ba893e2 > src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java > 41b9aab55aa439b13eba3545703ec97b44690444 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 925c025d21bf1d44e0c1d319f6653ecfa8899481 > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > a739bce1226d9435fa7d0b18e411064a4e78e49e > > Diff: https://reviews.apache.org/r/52074/diff/ > > > Testing > --- > > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 52074: switching from launchTask to acceptOffers
> On Sept. 20, 2016, 7:41 p.m., Mehrdad Nurolahzade wrote: > > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java, > > lines 52-56 > > <https://reviews.apache.org/r/52074/diff/3/?file=1505877#file1505877line52> > > > > We should start disallowing or limiting import static. It causes poor > > readability. > > > > I was looking for a makeTask() method on this file. Agreed. I have been told that Aurora's philosophy is convention first, so I'm following what has been set. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/#review149714 ----------- On Sept. 20, 2016, 7:14 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52074/ > --- > > (Updated Sept. 20, 2016, 7:14 p.m.) > > > Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer > Manji. > > > Bugs: AURORA-1776 > https://issues.apache.org/jira/browse/AURORA-1776 > > > Repository: aurora > > > Description > --- > > Dynamic reservation requires us to use acceptOffers so this is in preparation > of that work. acceptOffers is the call to dynamically reserve resources. > > > Diffs > - > > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java > 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a > src/main/java/org/apache/aurora/scheduler/mesos/Driver.java > 14481468926b82a9bad3ee78c85f176e8ba893e2 > src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java > 41b9aab55aa439b13eba3545703ec97b44690444 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 925c025d21bf1d44e0c1d319f6653ecfa8899481 > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > a739bce1226d9435fa7d0b18e411064a4e78e49e > > Diff: https://reviews.apache.org/r/52074/diff/ > > > Testing > --- > > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 52074: switching from launchTask to acceptOffers
> On Sept. 21, 2016, 5:54 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java, line 372 > > <https://reviews.apache.org/r/52074/diff/3/?file=1505876#file1505876line372> > > > > Can be inlined below. Done. > On Sept. 21, 2016, 5:54 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java, > > line 124 > > <https://reviews.apache.org/r/52074/diff/3/?file=1505875#file1505875line124> > > > > nit: add newline above Done. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/#review149862 ------- On Sept. 20, 2016, 7:14 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52074/ > --- > > (Updated Sept. 20, 2016, 7:14 p.m.) > > > Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer > Manji. > > > Bugs: AURORA-1776 > https://issues.apache.org/jira/browse/AURORA-1776 > > > Repository: aurora > > > Description > --- > > Dynamic reservation requires us to use acceptOffers so this is in preparation > of that work. acceptOffers is the call to dynamically reserve resources. > > > Diffs > - > > src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java > 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a > src/main/java/org/apache/aurora/scheduler/mesos/Driver.java > 14481468926b82a9bad3ee78c85f176e8ba893e2 > src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java > 41b9aab55aa439b13eba3545703ec97b44690444 > src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java > 925c025d21bf1d44e0c1d319f6653ecfa8899481 > src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java > a739bce1226d9435fa7d0b18e411064a4e78e49e > > Diff: https://reviews.apache.org/r/52074/diff/ > > > Testing > --- > > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 52074: switching from launchTask to acceptOffers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/ --- (Updated Sept. 20, 2016, 7:14 p.m.) Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer Manji. Bugs: AURORA-1776 https://issues.apache.org/jira/browse/AURORA-1776 Repository: aurora Description --- Dynamic reservation requires us to use acceptOffers so this is in preparation of that work. acceptOffers is the call to dynamically reserve resources. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 14481468926b82a9bad3ee78c85f176e8ba893e2 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 41b9aab55aa439b13eba3545703ec97b44690444 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 925c025d21bf1d44e0c1d319f6653ecfa8899481 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java a739bce1226d9435fa7d0b18e411064a4e78e49e Diff: https://reviews.apache.org/r/52074/diff/ Testing --- src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Dmitriy Shirchenko
Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52087/#review149707 --- Ship it! Ship It! - Dmitriy Shirchenko On Sept. 20, 2016, 6:57 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52087/ > --- > > (Updated Sept. 20, 2016, 6:57 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Zameer Manji. > > > Bugs: AURORA-1777 > https://issues.apache.org/jira/browse/AURORA-1777 > > > Repository: aurora > > > Description > --- > > Fix host maintenance commands to properly initialize the api client. > > > Diffs > - > > src/main/python/apache/aurora/admin/admin.py > 9fc89a2842d4651ac10aa82118531c450f051712 > src/main/python/apache/aurora/admin/admin_util.py > 394deb57af9ad8832a02ceab15f33b3c1e5c902b > src/main/python/apache/aurora/admin/host_maintenance.py > 677f870c50d1e19d34de36a3c677bb1dcd255ba0 > src/main/python/apache/aurora/admin/maintenance.py > bf446515222a1246b7fe32acd67315f8145a1212 > src/test/python/apache/aurora/admin/test_admin.py > f720742c50f774b9f5a41ba57ef251d08d79e8d1 > src/test/python/apache/aurora/admin/test_admin_sla.py > 54b5a823903b82c2082353ade132eb7b63e518db > src/test/python/apache/aurora/admin/test_admin_util.py > f5c8c69c1109d15ee3886fb863014c3285240db1 > src/test/python/apache/aurora/admin/util.py > d0a915cd8edee6b36d671b41f43a3af4fe751ae7 > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > e36726e0c0154e51bba40704e13eebec5c0f0d65 > > Diff: https://reviews.apache.org/r/52087/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > ran e2e tests. > > > Thanks, > > Joshua Cohen > >
Re: Review Request 52074: switching from launchTask to acceptOffers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/ --- (Updated Sept. 20, 2016, 6:08 p.m.) Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer Manji. Bugs: AURORA-1776 https://issues.apache.org/jira/browse/AURORA-1776 Repository: aurora Description --- Dynamic reservation requires us to use acceptOffers so this is in preparation of that work. acceptOffers is the call to dynamically reserve resources. Diffs (updated) - src/jmh/java/org/apache/aurora/benchmark/fakes/FakeDriver.java 1bfd8dbf3aec314aaeb89b756ea3a2049df8777a src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 14481468926b82a9bad3ee78c85f176e8ba893e2 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 41b9aab55aa439b13eba3545703ec97b44690444 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 925c025d21bf1d44e0c1d319f6653ecfa8899481 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java a739bce1226d9435fa7d0b18e411064a4e78e49e Diff: https://reviews.apache.org/r/52074/diff/ Testing --- src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Dmitriy Shirchenko
Re: Review Request 52087: Fix host maintenance commands to properly initialize the api client.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52087/#review149689 --- src/main/python/apache/aurora/admin/admin_util.py (line 272) <https://reviews.apache.org/r/52087/#comment217372> docstring, please! src/main/python/apache/aurora/admin/admin_util.py (line 273) <https://reviews.apache.org/r/52087/#comment217375> isinstance is preferred to type since isinstance keeps track of inheritance. src/main/python/apache/aurora/admin/admin_util.py (line 276) <https://reviews.apache.org/r/52087/#comment217376> is there a test for this switch? src/test/python/apache/aurora/admin/test_admin.py (line 232) <https://reviews.apache.org/r/52087/#comment217374> is there a mock with bypass_leader_redirect = True? - Dmitriy Shirchenko On Sept. 20, 2016, 5:32 p.m., Joshua Cohen wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52087/ > --- > > (Updated Sept. 20, 2016, 5:32 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Zameer Manji. > > > Repository: aurora > > > Description > --- > > Fix host maintenance commands to properly initialize the api client. > > > Diffs > - > > src/main/python/apache/aurora/admin/admin.py > 9fc89a2842d4651ac10aa82118531c450f051712 > src/main/python/apache/aurora/admin/admin_util.py > 394deb57af9ad8832a02ceab15f33b3c1e5c902b > src/main/python/apache/aurora/admin/host_maintenance.py > 677f870c50d1e19d34de36a3c677bb1dcd255ba0 > src/main/python/apache/aurora/admin/maintenance.py > bf446515222a1246b7fe32acd67315f8145a1212 > src/test/python/apache/aurora/admin/test_admin.py > f720742c50f774b9f5a41ba57ef251d08d79e8d1 > src/test/python/apache/aurora/admin/test_admin_sla.py > 54b5a823903b82c2082353ade132eb7b63e518db > src/test/python/apache/aurora/admin/util.py > d0a915cd8edee6b36d671b41f43a3af4fe751ae7 > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > e36726e0c0154e51bba40704e13eebec5c0f0d65 > > Diff: https://reviews.apache.org/r/52087/diff/ > > > Testing > --- > > ./build-support/jenkins/build.sh > ran e2e tests. > > > Thanks, > > Joshua Cohen > >
Review Request 52074: switching from launchTask to acceptOffers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52074/ --- Review request for Aurora, Maxim Khutornenko, Mehrdad Nurolahzade, and Zameer Manji. Bugs: AURORA-1776 https://issues.apache.org/jira/browse/AURORA-1776 Repository: aurora Description --- Dynamic reservation requires us to use acceptOffers so this is in preparation of that work. acceptOffers is the call to dynamically reserve resources. Diffs - src/main/java/org/apache/aurora/scheduler/mesos/Driver.java 14481468926b82a9bad3ee78c85f176e8ba893e2 src/main/java/org/apache/aurora/scheduler/mesos/SchedulerDriverService.java 41b9aab55aa439b13eba3545703ec97b44690444 src/main/java/org/apache/aurora/scheduler/offers/OfferManager.java 925c025d21bf1d44e0c1d319f6653ecfa8899481 src/test/java/org/apache/aurora/scheduler/offers/OfferManagerImplTest.java a739bce1226d9435fa7d0b18e411064a4e78e49e Diff: https://reviews.apache.org/r/52074/diff/ Testing --- src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh Thanks, Dmitriy Shirchenko
Re: Review Request 51980: Refactor of Webhook and no longer posting entire task state via webhook on scheduler restart
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51980/#review149530 --- @ReviewBot retry - Dmitriy Shirchenko On Sept. 19, 2016, 6:28 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51980/ > --- > > (Updated Sept. 19, 2016, 6:28 p.m.) > > > Review request for Aurora, Maxim Khutornenko, Stephan Erb, and Zameer Manji. > > > Bugs: AURORA-1772 > https://issues.apache.org/jira/browse/AURORA-1772 > > > Repository: aurora > > > Description > --- > > This is a refactor with addition of HttpClient injected into Webhook class as > opposed to previous usage of lower level HtttpURLConnection objects. > Additionally due to peformance issues, it is unncessary to POST entire task > state to webhook endpoint on every scheduler restart so that is removed in > this patch. > > > Diffs > - > > build.gradle d5a3a7a3bdb8349de6fc01d4a6271b32d942e531 > src/main/java/org/apache/aurora/scheduler/events/Webhook.java > e54aa19d67278fcb5586f07c399f09062f845a18 > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java > e4193f7518e66d210a6d5aae22a9f04e2d4984b3 > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > eaa70533a64740c74cebf15739b7142e2d3a4d11 > src/main/resources/org/apache/aurora/scheduler/webhook.json > 00787985510d7d415b8074bef06d28b635c78b09 > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > 488eefd14c3e67a41a75c809397c8d19f83cc08a > > Diff: https://reviews.apache.org/r/51980/diff/ > > > Testing > --- > > Part of reason for refactor is to allow easier testing so cleaner unit tests > were added with more code coverage. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 52049: Clarifying documentation for new contributors by adding a step for them to ask for their JIRA id to get whitelisted
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52049/ --- (Updated Sept. 19, 2016, 6:42 p.m.) Review request for Aurora, Joshua Cohen, Jake Farrell, and Zameer Manji. Repository: aurora Description --- Due to spam bots, JIRA accounts need to be whitelisted before they can self-assign tasks. Diffs (updated) - CONTRIBUTING.md 13ebbe4be281f869d276ebe97bfae75123899cfb Diff: https://reviews.apache.org/r/52049/diff/ Testing --- Thanks, Dmitriy Shirchenko
Review Request 52049: Clarifying documentation for new contributors by adding a step for them to ask for their JIRA id to get whitelisted
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52049/ --- Review request for Aurora, Joshua Cohen, Jake Farrell, and Zameer Manji. Repository: aurora Description --- Due to spam bots, JIRA accounts need to be whitelisted before they can self-assign tasks. Diffs - CONTRIBUTING.md 13ebbe4be281f869d276ebe97bfae75123899cfb Diff: https://reviews.apache.org/r/52049/diff/ Testing --- Thanks, Dmitriy Shirchenko
Re: Review Request 51980: Refactor of Webhook and no longer posting entire task state via webhook on scheduler restart
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51980/ --- (Updated Sept. 19, 2016, 6:28 p.m.) Review request for Aurora, Maxim Khutornenko, Stephan Erb, and Zameer Manji. Bugs: AURORA-1772 https://issues.apache.org/jira/browse/AURORA-1772 Repository: aurora Description --- This is a refactor with addition of HttpClient injected into Webhook class as opposed to previous usage of lower level HtttpURLConnection objects. Additionally due to peformance issues, it is unncessary to POST entire task state to webhook endpoint on every scheduler restart so that is removed in this patch. Diffs (updated) - build.gradle d5a3a7a3bdb8349de6fc01d4a6271b32d942e531 src/main/java/org/apache/aurora/scheduler/events/Webhook.java e54aa19d67278fcb5586f07c399f09062f845a18 src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java e4193f7518e66d210a6d5aae22a9f04e2d4984b3 src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java eaa70533a64740c74cebf15739b7142e2d3a4d11 src/main/resources/org/apache/aurora/scheduler/webhook.json 00787985510d7d415b8074bef06d28b635c78b09 src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 488eefd14c3e67a41a75c809397c8d19f83cc08a Diff: https://reviews.apache.org/r/51980/diff/ Testing --- Part of reason for refactor is to allow easier testing so cleaner unit tests were added with more code coverage. Thanks, Dmitriy Shirchenko
Re: Review Request 51980: Refactor of Webhook and no longer posting entire task state via webhook on scheduler restart
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51980/ --- (Updated Sept. 17, 2016, 1:40 a.m.) Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-1772 https://issues.apache.org/jira/browse/AURORA-1772 Repository: aurora Description (updated) --- This is a refactor with addition of HttpClient injected into Webhook class as opposed to previous usage of lower level HtttpURLConnection objects. Additionally due to peformance issues, it is unncessary to POST entire task state to webhook endpoint on every scheduler restart so that is removed in this patch. Diffs - build.gradle d5a3a7a3bdb8349de6fc01d4a6271b32d942e531 src/main/java/org/apache/aurora/scheduler/events/Webhook.java e54aa19d67278fcb5586f07c399f09062f845a18 src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java e4193f7518e66d210a6d5aae22a9f04e2d4984b3 src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java eaa70533a64740c74cebf15739b7142e2d3a4d11 src/main/resources/org/apache/aurora/scheduler/webhook.json 00787985510d7d415b8074bef06d28b635c78b09 src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java 488eefd14c3e67a41a75c809397c8d19f83cc08a Diff: https://reviews.apache.org/r/51980/diff/ Testing --- Part of reason for refactor is to allow easier testing so cleaner unit tests were added with more code coverage. Thanks, Dmitriy Shirchenko
Re: Review Request 50826: Populate the source field of ExecutorInfo.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50826/#review144853 --- Ship it! plz add a test and then LGTM - Dmitriy Shirchenko On Aug. 5, 2016, 12:34 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50826/ > --- > > (Updated Aug. 5, 2016, 12:34 a.m.) > > > Review request for Aurora, Joshua Cohen and Dmitriy Shirchenko. > > > Bugs: AURORA-1745 > https://issues.apache.org/jira/browse/AURORA-1745 > > > Repository: aurora > > > Description > --- > > b912e17 stopped populating the source field of the executor. For backwards > compatibility we should continue to populate this field and the `source` > label. > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.java > 34134430063b2d24a4e20d3f91ab899604edaf89 > > Diff: https://reviews.apache.org/r/50826/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50685: Support TBinaryProtocol over HTTP
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50685/#review144653 --- Ship it! Ship It! - Dmitriy Shirchenko On Aug. 3, 2016, 12:10 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50685/ > --- > > (Updated Aug. 3, 2016, 12:10 a.m.) > > > Review request for Aurora, Joshua Cohen, Maxim Khutornenko, and Dmitriy > Shirchenko. > > > Bugs: AURORA-1743 > https://issues.apache.org/jira/browse/AURORA-1743 > > > Repository: aurora > > > Description > --- > > This replaces the `TServlet` servlet from thrift with our own servlet which > dispatches thrift responses based on the content type of the request. This > enables a client to use either the thrift json protocol or the binary > protocol when communicating with the scheduler. > > Without this patch the current behaviour is: > - Regardless of content type of the request, assume the request is json and > return json with a `application/x-thrift` content type. > > With this patch the behaviour becomes: > - A request with no content type is assumed to be json and returns json with > a `application/vnd.apache.thrift.json` content type. This maintains backwards > compatability with clients that may not set a content type. > - A request with a content type of `application/x-thrift` is assumed to be > json and returns json with a `application/vnd.apache.thrift.json` content > type. This maintains backwards compatability with the client. > - A request with a content type of `application/json` returns json with a > `application/vnd.apache.thrift.json` content type. This maintains backwards > compatability with the AJAX UI. > - A request with a content type of `application/vnd.apache.thrift.json` > returns json with a `application/vnd.apache.thrift.json` content type. > - A request with a content type of `application/vnd.apache.thrift.binary` > returns binary thrift with a `application/vnd.apache.thrift.json` content > type. > - Any other content type in a request results in a HTTP 415 response. > > > Diffs > - > > RELEASE-NOTES.md d46420ef876b88d9ab68c4816f1c2d6780aba702 > src/main/java/org/apache/aurora/scheduler/http/api/ApiModule.java > cd5adf9655dc3368dbe06bfee15c65182176be2e > > src/main/java/org/apache/aurora/scheduler/http/api/TContentAwareServlet.java > PRE-CREATION > src/test/java/org/apache/aurora/scheduler/http/api/ApiIT.java > 31f5cb3bed48eef60c3b2becb2ed285e93f2bd5a > > src/test/java/org/apache/aurora/scheduler/http/api/TContentAwareServletTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/50685/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50478: Improve `executorLost` error message.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50478/#review143764 --- Ship it! Ship It! - Dmitriy Shirchenko On July 27, 2016, 1:25 a.m., Zameer Manji wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50478/ > --- > > (Updated July 27, 2016, 1:25 a.m.) > > > Review request for Aurora, Joshua Cohen and Dmitriy Shirchenko. > > > Repository: aurora > > > Description > --- > > Improve `executorLost` error message by including the slave id. > > > Diffs > - > > src/main/java/org/apache/aurora/scheduler/mesos/MesosSchedulerImpl.java > 433fc904e419b533f879ad34284ce043118f33d4 > > Diff: https://reviews.apache.org/r/50478/diff/ > > > Testing > --- > > > Thanks, > > Zameer Manji > >
Re: Review Request 50052: AURORA-1736 Display reservations and persistent volumes in /offers debug http endpoint
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50052/#review142830
---
File Attachment: NEW - offers-new.json
<https://reviews.apache.org//r/50052/#fcomment102>
Why is there a double nested labels: labels array under reservation? Is
this just a carry over from how Aurora gets offers from Mesos?
So there is a key `labels` and then underneath is there's another key
`labels` whose value is an array. Seems like one of them is not necessary.
```json
reservation: {
labels: {
labels: [
{
key: "job",
value: "devcluster/www-data/prod/hello"
}
]
}
}
```
- Dmitriy Shirchenko
On July 19, 2016, 2:12 a.m., Mehrdad Nurolahzade wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50052/
> ---
>
> (Updated July 19, 2016, 2:12 a.m.)
>
>
> Review request for Aurora, Joshua Cohen and Stephan Erb.
>
>
> Bugs: AURORA-1736
> https://issues.apache.org/jira/browse/AURORA-1736
>
>
> Repository: aurora
>
>
> Description
> ---
>
> AURORA-1736 Display reservations and persistent volumes in /offers debug http
> endpoint
>
>
> Diffs
> -
>
> config/legacy_untested_classes.txt 1ea2183ab20cc5c6bca147bcea4e5c708d576b62
> src/main/java/org/apache/aurora/scheduler/http/Offers.java
> 80f082410896a50d86c7886736caf79581f5051c
> src/test/java/org/apache/aurora/scheduler/http/OffersTest.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/50052/diff/
>
>
> Testing
> ---
>
> Manual, Jenkins, and end_to_end
>
>
> File Attachments
>
>
> CURRENT
>
> https://reviews.apache.org/media/uploaded/files/2016/07/18/1de4c357-c932-4c84-962f-4209a5b679bc__offers-old.json
> NEW
>
> https://reviews.apache.org/media/uploaded/files/2016/07/19/799bcd1f-f9c8-4b6e-bbaa-ce8022b1dac1__offers-new.json
>
>
> Thanks,
>
> Mehrdad Nurolahzade
>
>
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
> On June 4, 2016, 5:17 p.m., Stephan Erb wrote: > > docs/features/webhooks.md, line 1 > > <https://reviews.apache.org/r/47440/diff/11/?file=1406242#file1406242line1> > > > > Please add a reference to this file in `docs/README.md` Done. > On June 4, 2016, 5:17 p.m., Stephan Erb wrote: > > docs/features/webhooks.md, line 8 > > <https://reviews.apache.org/r/47440/diff/11/?file=1406242#file1406242line8> > > > > Please also add an example event payload that one would receive via a > > post request. Done. > On June 4, 2016, 5:17 p.m., Stephan Erb wrote: > > src/main/resources/org/apache/aurora/scheduler/webhook.json, lines 1-8 > > <https://reviews.apache.org/r/47440/diff/11/?file=1406249#file1406249line1> > > > > I still think we should get rid of that file. It is not really a > > usuable by default for anyone, shipping it in our source and binary > > distribution therefore does not make much sense. It's used for testing, so I prefer to keep it here. It's on par with how tier_config file is shipped. > On June 4, 2016, 5:17 p.m., Stephan Erb wrote: > > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java, lines > > 52-65 > > <https://reviews.apache.org/r/47440/diff/11/?file=1406250#file1406250line52> > > > > I have to admit, that I don't really understand what those tests are > > _actually_ asserting, especially given the coarse-grained > > `try-catch`-clauses in the code. > > > > Personally, I would have opted to start a proper webserver within my > > test that actually asserts data is received as requested. I have never > > done this in Java, but it seems possible (for example with Jetty): > > https://stackoverflow.com/questions/29758607/how-to-run-jetty-server-for-java-junit-testing Tests are asserting that functions got called with expected arguments. Great idea. I have never setup a test server in Java either, so maybe we can rely on unit tests. > On June 4, 2016, 5:17 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, lines 71-73 > > <https://reviews.apache.org/r/47440/diff/11/?file=1406246#file1406246line71> > > > > Seeing the code spelled out here, this looks kind of complicated. > > > > I believe you can make the code more obvious if you don't do any error > > handling within `initializeConnection` but instead just let the exceptions > > bubble-up and handle them in the first `try`-block in `callEndpoint`. You > > might even consider merging all try-blocks into just a single one. I try to break up code into more digestable chunks so I think I prefer isolating parts and letting `initializeConnection` dealing with object creation, and `callEndpoint` with actual call. I see what you mean though. > On June 4, 2016, 5:17 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 82 > > <https://reviews.apache.org/r/47440/diff/11/?file=1406246#file1406246line82> > > > > According to the docs, `getErrorStream` _may_ return `null`, so we need > > a guard here. > > https://docs.oracle.com/javase/7/docs/api/java/net/HttpURLConnection.html#getErrorStream() Good call. Done. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/#review136169 --- On June 4, 2016, 6:33 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47440/ > --- > > (Updated June 4, 2016, 6:33 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Stephan Erb. > > > Bugs: AURORA-1683 > https://issues.apache.org/jira/browse/AURORA-1683 > > > Repository: aurora > > > Description > --- > > Looking for some feedback whether I'm on the correct path in adding a > webhook. All comments are welcome! > > > Diffs > - > > RELEASE-NOTES.md 21a141f1acadfdaa94247b75de2f734b11868137 > docs/README.md aea8518b07918ae1cd3322873e72fc8bfcd6df7b > docs/features/webhooks.md PRE-CREATION > docs/reference/scheduler-configuration.md > f7d676d0ed6bc536f4341dbb9365cf50e8607efb > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 9ebfe230836e88a97bc60092373f72f176a8f6f2 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 2a4c0665e48d30e0655de00bd
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated June 4, 2016, 6:33 p.m.) Review request for Aurora, Maxim Khutornenko and Stephan Erb. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - RELEASE-NOTES.md 21a141f1acadfdaa94247b75de2f734b11868137 docs/README.md aea8518b07918ae1cd3322873e72fc8bfcd6df7b docs/features/webhooks.md PRE-CREATION docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated June 3, 2016, 11:54 p.m.) Review request for Aurora, Maxim Khutornenko and Stephan Erb. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - RELEASE-NOTES.md 21a141f1acadfdaa94247b75de2f734b11868137 docs/features/webhooks.md PRE-CREATION docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated June 3, 2016, 10:17 p.m.) Review request for Aurora, Maxim Khutornenko and Stephan Erb. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - RELEASE-NOTES.md 4cbf92e6556d4d84053292e26f65755d971089c0 docs/features/webhooks.md PRE-CREATION docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
> On May 27, 2016, 2 p.m., Stephan Erb wrote: > > Repeating from my previous review: Would be great if you could add > > > > * an entry to the release notes > > * a minimal docs/features/webhooks.md that shows a valid webhook config and > > how a dispatched event would look like. Done. > On May 27, 2016, 2 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java, line 71 > > <https://reviews.apache.org/r/47440/diff/9/?file=1395863#file1395863line71> > > > > Is it intentional that you use multiple names for the timeout here? > > (`connectTimeout` vs `timeoutMsec`) Yea, I followed Maxim's suggestion to make it more clear that the timeout value is in milliseconds, and connectTimeout corresponds to the value `HttpURLConnection` expects. How does that sound? Or still too confusing? > On May 27, 2016, 2 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 72 > > <https://reviews.apache.org/r/47440/diff/9/?file=1395862#file1395862line72> > > > > This will fail with a `NullPointerException` when > > `initializeConnection()` returns `None`. Same applies to other places in > > the content of the try block. Done. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/#review135233 --- On June 3, 2016, 10:17 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47440/ > --- > > (Updated June 3, 2016, 10:17 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Stephan Erb. > > > Bugs: AURORA-1683 > https://issues.apache.org/jira/browse/AURORA-1683 > > > Repository: aurora > > > Description > --- > > Looking for some feedback whether I'm on the correct path in adding a > webhook. All comments are welcome! > > > Diffs > - > > RELEASE-NOTES.md 4cbf92e6556d4d84053292e26f65755d971089c0 > docs/features/webhooks.md PRE-CREATION > docs/reference/scheduler-configuration.md > f7d676d0ed6bc536f4341dbb9365cf50e8607efb > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 9ebfe230836e88a97bc60092373f72f176a8f6f2 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc > src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47440/diff/ > > > Testing > --- > > Need to fix tests. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
> On May 26, 2016, 10:01 p.m., Maxim Khutornenko wrote: > > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java, line 46 > > <https://reviews.apache.org/r/47440/diff/8/?file=1395856#file1395856line46> > > > > kill newline Done. > On May 26, 2016, 10:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java, lines > > 81-82 > > <https://reviews.apache.org/r/47440/diff/8/?file=1395854#file1395854line81> > > > > +4 more spaces Done. > On May 26, 2016, 10:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java, line 58 > > <https://reviews.apache.org/r/47440/diff/8/?file=1395854#file1395854line58> > > > > Should be enough to `this(WEBHOOK_CONFIG_FILE.hasAppliedValue() && > > WEBHOOK_CONFIG_FILE.hasAppliedValue()` as `@Exists` already does file > > validation for you. Done. > On May 26, 2016, 10:01 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, lines 73-74 > > <https://reviews.apache.org/r/47440/diff/8/?file=1395852#file1395852line73> > > > > -4 spaces Done. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/#review135087 --- On May 26, 2016, 10:29 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47440/ > --- > > (Updated May 26, 2016, 10:29 p.m.) > > > Review request for Aurora, Maxim Khutornenko and Stephan Erb. > > > Bugs: AURORA-1683 > https://issues.apache.org/jira/browse/AURORA-1683 > > > Repository: aurora > > > Description > --- > > Looking for some feedback whether I'm on the correct path in adding a > webhook. All comments are welcome! > > > Diffs > - > > docs/reference/scheduler-configuration.md > f7d676d0ed6bc536f4341dbb9365cf50e8607efb > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 9ebfe230836e88a97bc60092373f72f176a8f6f2 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc > src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47440/diff/ > > > Testing > --- > > Need to fix tests. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated May 26, 2016, 10:29 p.m.) Review request for Aurora, Maxim Khutornenko and Stephan Erb. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated May 26, 2016, 9:48 p.m.) Review request for Aurora, Maxim Khutornenko and Stephan Erb. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb examples/vagrant/upstart/aurora-scheduler.conf 3d9e706de564df5e24cb34265bebc0db1cad11a0 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated May 26, 2016, 9:14 p.m.) Review request for Aurora, Maxim Khutornenko and Stephan Erb. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb examples/vagrant/upstart/aurora-scheduler.conf 3d9e706de564df5e24cb34265bebc0db1cad11a0 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/#review134853 --- src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 47) <https://reviews.apache.org/r/47440/#comment199792> Done. src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 91) <https://reviews.apache.org/r/47440/#comment199787> Done. src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 34) <https://reviews.apache.org/r/47440/#comment199791> Done. src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 65) <https://reviews.apache.org/r/47440/#comment199790> Done? src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 67) <https://reviews.apache.org/r/47440/#comment199788> Done. src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 75) <https://reviews.apache.org/r/47440/#comment199789> Done, - Dmitriy Shirchenko On May 25, 2016, 1:02 a.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47440/ > --- > > (Updated May 25, 2016, 1:02 a.m.) > > > Review request for Aurora, Maxim Khutornenko and Stephan Erb. > > > Bugs: AURORA-1683 > https://issues.apache.org/jira/browse/AURORA-1683 > > > Repository: aurora > > > Description > --- > > Looking for some feedback whether I'm on the correct path in adding a > webhook. All comments are welcome! > > > Diffs > - > > docs/reference/scheduler-configuration.md > f7d676d0ed6bc536f4341dbb9365cf50e8607efb > examples/vagrant/upstart/aurora-scheduler.conf > 3d9e706de564df5e24cb34265bebc0db1cad11a0 > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 9ebfe230836e88a97bc60092373f72f176a8f6f2 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc > src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47440/diff/ > > > Testing > --- > > Need to fix tests. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
> On May 23, 2016, 6 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 39 > > <https://reviews.apache.org/r/47440/diff/5/?file=1387602#file1387602line39> > > > > Can your return this from `initializeConnection()` instead of keeping a > > global field? Done. > On May 23, 2016, 6 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 47 > > <https://reviews.apache.org/r/47440/diff/5/?file=1387602#file1387602line47> > > > > This can be moved inside try. Done. > On May 23, 2016, 6 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 38 > > <https://reviews.apache.org/r/47440/diff/5/?file=1387602#file1387602line38> > > > > Would it make sense to move this into WebhookInfo instead? Done. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/#review134392 --- On May 25, 2016, 1:02 a.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47440/ > --- > > (Updated May 25, 2016, 1:02 a.m.) > > > Review request for Aurora, Maxim Khutornenko and Stephan Erb. > > > Bugs: AURORA-1683 > https://issues.apache.org/jira/browse/AURORA-1683 > > > Repository: aurora > > > Description > --- > > Looking for some feedback whether I'm on the correct path in adding a > webhook. All comments are welcome! > > > Diffs > - > > docs/reference/scheduler-configuration.md > f7d676d0ed6bc536f4341dbb9365cf50e8607efb > examples/vagrant/upstart/aurora-scheduler.conf > 3d9e706de564df5e24cb34265bebc0db1cad11a0 > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 9ebfe230836e88a97bc60092373f72f176a8f6f2 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc > src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47440/diff/ > > > Testing > --- > > Need to fix tests. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47440/#review134637
---
docs/reference/scheduler-configuration.md (line 222)
<https://reviews.apache.org/r/47440/#comment199490>
Done.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 42)
<https://reviews.apache.org/r/47440/#comment199500>
Done.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 46)
<https://reviews.apache.org/r/47440/#comment199479>
Done.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (lines 54 - 55)
<https://reviews.apache.org/r/47440/#comment199480>
Done.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 58)
<https://reviews.apache.org/r/47440/#comment199533>
Done.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 59)
<https://reviews.apache.org/r/47440/#comment199481>
Kafka Proxy requires this to be here. I also didn't see harm in adding it.
It's not a straitforward header as it's dynamic but if you want, I can make it
optional by adding to headers config with a key:value to be filled in later in
code
```
{
"headers": {
"Content-Type": "application/vnd.kafka.json.v1+json",
"Producer-Type": "reliable",
"Timestamp": "Timestamp"
},
"targetURL": "http://localhost:5000/;
}
```
And then add a property if it's configured. How does that sound? Or we can
keep it as is. LMK.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 60)
<https://reviews.apache.org/r/47440/#comment199532>
Removing. This looks to be unncessary.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 71)
<https://reviews.apache.org/r/47440/#comment199482>
Done.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (line 80)
<https://reviews.apache.org/r/47440/#comment199483>
Done.
src/main/java/org/apache/aurora/scheduler/events/Webhook.java (lines 82 - 84)
<https://reviews.apache.org/r/47440/#comment199484>
Yea, they are needed to close the connection. Otherwise connection stays
open waiting for input to be read.
I added inline close statements.
src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 27)
<https://reviews.apache.org/r/47440/#comment199487>
Done.
src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java (line 42)
<https://reviews.apache.org/r/47440/#comment199489>
Done.
src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java (line 45)
<https://reviews.apache.org/r/47440/#comment199486>
It's in a separate path eg tiers.json. Is that OK? It's not mixed in with
the code.
src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java (line 73)
<https://reviews.apache.org/r/47440/#comment199485>
Done.
- Dmitriy Shirchenko
On May 22, 2016, 1:29 p.m., Dmitriy Shirchenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47440/
> ---
>
> (Updated May 22, 2016, 1:29 p.m.)
>
>
> Review request for Aurora, Maxim Khutornenko and Stephan Erb.
>
>
> Bugs: AURORA-1683
> https://issues.apache.org/jira/browse/AURORA-1683
>
>
> Repository: aurora
>
>
> Description
> ---
>
> Looking for some feedback whether I'm on the correct path in adding a
> webhook. All comments are welcome!
>
>
> Diffs
> -
>
> docs/reference/scheduler-configuration.md
> f7d676d0ed6bc536f4341dbb9365cf50e8607efb
> examples/vagrant/upstart/aurora-scheduler.conf
> 3d9e706de564df5e24cb34265bebc0db1cad11a0
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
> 9ebfe230836e88a97bc60092373f72f176a8f6f2
> src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java
> 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java
> PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java
> PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java
> PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47440/diff/
>
>
> Testing
> ---
>
> Need to fix tests.
>
>
> Thanks,
>
> Dmitriy Shirchenko
>
>
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
> On May 18, 2016, 10 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java, line 45 > > <https://reviews.apache.org/r/47440/diff/4/?file=1387379#file1387379line45> > > > > I am surprised to find this one here. Are you planning to bundle a > > default config file? This is mainly for tests and as a sample to start with. If no user specified file is present this isn't used. > On May 18, 2016, 10 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 83 > > <https://reviews.apache.org/r/47440/diff/4/?file=1387377#file1387377line83> > > > > What's that line doing? Done. Fixing with recommended approach in https://docs.oracle.com/javase/7/docs/technotes/guides/net/http-keepalive.html > On May 18, 2016, 10 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 75 > > <https://reviews.apache.org/r/47440/diff/4/?file=1387377#file1387377line75> > > > > Is that an outdated comment? It is. Removing. Thanks. > On May 18, 2016, 10 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 39 > > <https://reviews.apache.org/r/47440/diff/4/?file=1387377#file1387377line39> > > > > Thinking out loud: Do we have to worry about synchronization? Or is > > there a guarantee that only one event will be dispatched at a time? > > > > If I understand this > > https://docs.oracle.com/javase/7/docs/technotes/guides/net/http-keepalive.html > > and that > > https://stackoverflow.com/questions/16256681/how-to-reuse-httpurlconnection?lq=1 > > correctly, we could simply re-create a connection object each time. Java > > should do the right thing behind the scenes. This would relieve us from any > > synchronization needs. Yea, that's my understanding as well ie we don't have to worry about synchronization since HttpURLConnection is recreated each time taskChangedState is triggered. > On May 18, 2016, 10 p.m., Stephan Erb wrote: > > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java, line 163 > > <https://reviews.apache.org/r/47440/diff/4/?file=1387376#file1387376line163> > > > > By using the object as-is, we have effectively made in internal API > > public. That could prevent us from doing necessary changes in the future, > > as we would have to adhere to a deprecation cycle. It might therefore make > > sense to establish an explicit schema. Good point. Essentially these are IScheduledTask objects which I believe already require a deprecation cycle. Is this correct? - Dmitriy ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/#review133819 --- On May 18, 2016, 9:12 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47440/ > --- > > (Updated May 18, 2016, 9:12 p.m.) > > > Review request for Aurora. > > > Bugs: AURORA-1683 > https://issues.apache.org/jira/browse/AURORA-1683 > > > Repository: aurora > > > Description > --- > > Looking for some feedback whether I'm on the correct path in adding a > webhook. All comments are welcome! > > > Diffs > - > > docs/reference/scheduler-configuration.md > f7d676d0ed6bc536f4341dbb9365cf50e8607efb > examples/vagrant/upstart/aurora-scheduler.conf > 3d9e706de564df5e24cb34265bebc0db1cad11a0 > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 9ebfe230836e88a97bc60092373f72f176a8f6f2 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc > src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47440/diff/ > > > Testing > --- > > Need to fix tests. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated May 19, 2016, 2:16 a.m.) Review request for Aurora. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb examples/vagrant/upstart/aurora-scheduler.conf 3d9e706de564df5e24cb34265bebc0db1cad11a0 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
? Yea, good idea! > On May 17, 2016, 5:09 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 45 > > <https://reviews.apache.org/r/47440/diff/3/?file=1385244#file1385244line45> > > > > +1 > > > > Also, consider caching and reusing the connection in a `LoadingCache` > > [1] with `expireAfterAccess` set to close it due to inactivity. You can > > implementing disconnect() inside a `RemovalListener`. > > > > [1] - https://github.com/google/guava/wiki/CachesExplained Can you help me and clarify what we are trying to optimize here? I looked at https://docs.oracle.com/javase/8/docs/technotes/guides/net/http-keepalive.html and if I'm reading that correctly, at a layer below HTTPURLConnection, the actual connections are persisted. I added a Keep-Alive and removed disconnect. > On May 17, 2016, 5:09 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 30 > > <https://reviews.apache.org/r/47440/diff/3/?file=1385244#file1385244line30> > > > > Missing javadoc for class and public methods. Done. > On May 17, 2016, 5:09 p.m., Maxim Khutornenko wrote: > > docs/reference/scheduler-configuration.md, line 84 > > <https://reviews.apache.org/r/47440/diff/3/?file=1385240#file1385240line84> > > > > Formatting is off here and below. Done. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/#review133555 --- On May 18, 2016, 9:12 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47440/ > --- > > (Updated May 18, 2016, 9:12 p.m.) > > > Review request for Aurora. > > > Bugs: AURORA-1683 > https://issues.apache.org/jira/browse/AURORA-1683 > > > Repository: aurora > > > Description > --- > > Looking for some feedback whether I'm on the correct path in adding a > webhook. All comments are welcome! > > > Diffs > - > > docs/reference/scheduler-configuration.md > f7d676d0ed6bc536f4341dbb9365cf50e8607efb > examples/vagrant/upstart/aurora-scheduler.conf > 3d9e706de564df5e24cb34265bebc0db1cad11a0 > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 9ebfe230836e88a97bc60092373f72f176a8f6f2 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc > src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47440/diff/ > > > Testing > --- > > Need to fix tests. > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated May 18, 2016, 9:12 p.m.) Review request for Aurora. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb examples/vagrant/upstart/aurora-scheduler.conf 3d9e706de564df5e24cb34265bebc0db1cad11a0 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
> On May 17, 2016, 2:51 a.m., George Sirois wrote:
> > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 86
> > <https://reviews.apache.org/r/47440/diff/1/?file=1385132#file1385132line86>
> >
> > Should this be dispatched on another thread or added to a queue for
> > future delivery? AFAICT the distribution of events is synchronous, so it
> > seems like operations that might block for any significant amount of time
> > could cause issues.
>
> Dmitriy Shirchenko wrote:
> Yea, I'm not sure. Do you have examples of best practices on how to
> dispatch to another thread?
I looked at the logs, and looks like each event is sent via own process judging
by logs.
Do you think this is correct or I'm off base?
```
I0517 06:28:28.250 [AsyncProcessor-6, Webhook:57] Sending message
{"task":{"cachedHashCode":0,"assignedTask":{"cachedHashCode":0,"taskId":"vagrant-test-http_e
I0517 06:28:28.254 [AsyncProcessor-6, Webhook:77] Disconnecting
I0517 06:28:28.256 [AsyncProcessor-6, Webhook:80] Done with call
I0517 06:28:28.808 [AsyncProcessor-4, Webhook:57] Sending message
{"task":{"cachedHashCode":0,"assignedTask":{"cachedHashCode":0,"taskId":"vagrant-test-http_e
xample-0-81ba7ce9-1900-4177-98af-847ae3a90fb7","slaveId":"8639d42c-2c68-40d9-9131-43e597457de5-S0","slaveHost":"192.168.33.7","task":{"cachedHashCode":-528474
I0517 06:28:28.810 [AsyncProcessor-4, Webhook:77] Disconnecting
I0517 06:28:28.810 [AsyncProcessor-4, Webhook:80] Done with call
```
- Dmitriy
-------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47440/#review133482
---
On May 17, 2016, 6:44 a.m., Dmitriy Shirchenko wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47440/
> ---
>
> (Updated May 17, 2016, 6:44 a.m.)
>
>
> Review request for Aurora.
>
>
> Bugs: AURORA-1683
> https://issues.apache.org/jira/browse/AURORA-1683
>
>
> Repository: aurora
>
>
> Description
> ---
>
> Looking for some feedback whether I'm on the correct path in adding a
> webhook. All comments are welcome!
>
>
> Diffs
> -
>
> docs/reference/scheduler-configuration.md
> f7d676d0ed6bc536f4341dbb9365cf50e8607efb
> examples/vagrant/upstart/aurora-scheduler.conf
> 3d9e706de564df5e24cb34265bebc0db1cad11a0
> src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java
> 9ebfe230836e88a97bc60092373f72f176a8f6f2
> src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java
> 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc
> src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java
> PRE-CREATION
> src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java
> PRE-CREATION
> src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION
> src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java
> PRE-CREATION
>
> Diff: https://reviews.apache.org/r/47440/diff/
>
>
> Testing
> ---
>
> Need to fix tests.
>
>
> Thanks,
>
> Dmitriy Shirchenko
>
>
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated May 17, 2016, 6:44 a.m.) Review request for Aurora. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb examples/vagrant/upstart/aurora-scheduler.conf 3d9e706de564df5e24cb34265bebc0db1cad11a0 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- (Updated May 17, 2016, 5:49 a.m.) Review request for Aurora. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs (updated) - docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb examples/vagrant/upstart/aurora-scheduler.conf 3d9e706de564df5e24cb34265bebc0db1cad11a0 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Re: Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
> On May 17, 2016, 2:51 a.m., George Sirois wrote: > > src/main/java/org/apache/aurora/scheduler/events/Webhook.java, line 86 > > <https://reviews.apache.org/r/47440/diff/1/?file=1385132#file1385132line86> > > > > Should this be dispatched on another thread or added to a queue for > > future delivery? AFAICT the distribution of events is synchronous, so it > > seems like operations that might block for any significant amount of time > > could cause issues. Yea, I'm not sure. Do you have examples of best practices on how to dispatch to another thread? > On May 17, 2016, 2:51 a.m., George Sirois wrote: > > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java, line 17 > > <https://reviews.apache.org/r/47440/diff/1/?file=1385133#file1385133line17> > > > > Should this structure be more generic (perhaps a list of key/values for > > headers)? It seems very tied to Kafka as-is. Good point. Yea, I will switch to a more generic config. Thanks. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/#review133482 ------- On May 17, 2016, 2:10 a.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47440/ > --- > > (Updated May 17, 2016, 2:10 a.m.) > > > Review request for Aurora. > > > Bugs: AURORA-1683 > https://issues.apache.org/jira/browse/AURORA-1683 > > > Repository: aurora > > > Description > --- > > Looking for some feedback whether I'm on the correct path in adding a > webhook. All comments are welcome! > > > Diffs > - > > docs/reference/scheduler-configuration.md > f7d676d0ed6bc536f4341dbb9365cf50e8607efb > examples/vagrant/upstart/aurora-scheduler.conf > 3d9e706de564df5e24cb34265bebc0db1cad11a0 > src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java > 9ebfe230836e88a97bc60092373f72f176a8f6f2 > src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java > 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc > src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java > PRE-CREATION > src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java > PRE-CREATION > src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION > src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/47440/diff/ > > > Testing > --- > > Need to fix tests. > > > Thanks, > > Dmitriy Shirchenko > >
Review Request 47440: RFC for WIP: adding webhook code that can POST events to an endpoint
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47440/ --- Review request for Aurora. Bugs: AURORA-1683 https://issues.apache.org/jira/browse/AURORA-1683 Repository: aurora Description --- Looking for some feedback whether I'm on the correct path in adding a webhook. All comments are welcome! Diffs - docs/reference/scheduler-configuration.md f7d676d0ed6bc536f4341dbb9365cf50e8607efb examples/vagrant/upstart/aurora-scheduler.conf 3d9e706de564df5e24cb34265bebc0db1cad11a0 src/main/java/org/apache/aurora/scheduler/app/SchedulerMain.java 9ebfe230836e88a97bc60092373f72f176a8f6f2 src/main/java/org/apache/aurora/scheduler/events/PubsubEvent.java 2a4c0665e48d30e0655de00bd7f6f9b49f01eafc src/main/java/org/apache/aurora/scheduler/events/Webhook.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookInfo.java PRE-CREATION src/main/java/org/apache/aurora/scheduler/events/WebhookModule.java PRE-CREATION src/main/resources/org/apache/aurora/scheduler/webhook.json PRE-CREATION src/test/java/org/apache/aurora/scheduler/events/WebhookTest.java PRE-CREATION Diff: https://reviews.apache.org/r/47440/diff/ Testing --- Need to fix tests. Thanks, Dmitriy Shirchenko
Review Request 46361: deleting --setuid-health-checks and switching to --nosetuid-health-checks flag to to control whether the executor runs health checks as the job role's user.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46361/ --- Review request for Aurora, Bill Farner and Zameer Manji. Repository: aurora Description --- deleting --setuid-health-checks and switching to --nosetuid-health-checks flag to to control whether the executor runs health checks as the job role's user. Diffs - RELEASE-NOTES.md 2068d9c141b79643f0c0687a0834a37b3af9955c src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 0b3c38c298ab54dce65d7b1dc4d965fdf833acc0 src/main/python/apache/aurora/executor/common/health_checker.py 88b629e944ab653e8acb9a8d20cdded125a8ac73 src/test/python/apache/aurora/executor/common/test_health_checker.py ff39e869634830b0eb59ddfe52a5a48ca1605a66 Diff: https://reviews.apache.org/r/46361/diff/ Testing --- Thanks, Dmitriy Shirchenko
Re: Review Request 46290: Adding a flag to control whether the executor runs health checks as the job's role's user
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 18, 2016, 7:02 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description --- Adding a flag to control whether the executor runs health checks as the job's role's user Diffs (updated) - RELEASE-NOTES.md 99d261b7928576f91d4b69bdc6be7210578ee7b3 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 46290: Adding a flag to control whether the executor runs health checks as the job's role's user
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 18, 2016, 7:01 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description --- Adding a flag to control whether the executor runs health checks as the job's role's user Diffs (updated) - RELEASE-NOTES.md 99d261b7928576f91d4b69bdc6be7210578ee7b3 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 46290: Adding a flag to control whether the executor runs health checks as the job's role's user
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 18, 2016, 6:43 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description --- Adding a flag to control whether the executor runs health checks as the job's role's user Diffs (updated) - RELEASE-NOTES.md 99d261b7928576f91d4b69bdc6be7210578ee7b3 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 46290: Adding a flag to control whether the executor runs health checks as the job's role's user
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 18, 2016, 6:42 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Summary (updated) - Adding a flag to control whether the executor runs health checks as the job's role's user Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description (updated) --- Adding a flag to control whether the executor runs health checks as the job's role's user Diffs - RELEASE-NOTES.md 99d261b7928576f91d4b69bdc6be7210578ee7b3 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 46290: adding --setuid-health-checks to not demote health check user:group to role
> On April 18, 2016, 6:33 p.m., Joshua Cohen wrote: > > There was a discussion in IRC about just configuring your health check to > > be `sudo ...`. I may have missed the resolution of that discussion, was > > that not a feasible approach? It is not because the actual shell command (with or without sudo) gets executed after ``` pwd.getpwnam ``` tries to get user that may not exist. Executor fails hard on that line. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/#review129401 --- On April 18, 2016, 6:34 p.m., Dmitriy Shirchenko wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46290/ > --- > > (Updated April 18, 2016, 6:34 p.m.) > > > Review request for Aurora, Bill Farner and Zameer Manji. > > > Bugs: AURORA-1666 > https://issues.apache.org/jira/browse/AURORA-1666 > > > Repository: aurora > > > Description > --- > > adding --setuid-health-checks flag to not demote health check user:group to > role > > > Diffs > - > > RELEASE-NOTES.md 99d261b7928576f91d4b69bdc6be7210578ee7b3 > src/main/python/apache/aurora/executor/bin/thermos_executor_main.py > 40a0cd6396a78c591debf5e2be11363ecf496231 > src/main/python/apache/aurora/executor/common/health_checker.py > d8675beb8f16ebdd2d6946367784411fe84a5cfc > src/test/python/apache/aurora/executor/common/test_health_checker.py > 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a > > Diff: https://reviews.apache.org/r/46290/diff/ > > > Testing > --- > > - end to end tests > - added unit tests > > > Thanks, > > Dmitriy Shirchenko > >
Re: Review Request 46290: adding --setuid-health-checks to not demote health check user:group to role
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 18, 2016, 6:34 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description --- adding --setuid-health-checks flag to not demote health check user:group to role Diffs (updated) - RELEASE-NOTES.md 99d261b7928576f91d4b69bdc6be7210578ee7b3 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 46290: adding --setuid-health-checks to not demote health check user:group to role
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 18, 2016, 6:33 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Summary (updated) - adding --setuid-health-checks to not demote health check user:group to role Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description (updated) --- adding --setuid-health-checks flag to not demote health check user:group to role Diffs - RELEASE-NOTES.md a0536ec352119952d4d58aa5f36e5e9a7b7d2e6e src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 46290: adding do-not-demote-health-check-to-role flag to not demote health check user:group to role
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 18, 2016, 6:30 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description --- adding do-not-demote-health-check-to-role flag to not demote health check user:group to role Diffs (updated) - RELEASE-NOTES.md a0536ec352119952d4d58aa5f36e5e9a7b7d2e6e src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 46290: adding do-not-demote-health-check-to-role flag to not demote health check user:group to role
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 15, 2016, 11:59 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description --- adding do-not-demote-health-check-to-role flag to not demote health check user:group to role Diffs (updated) - RELEASE-NOTES.md a0536ec352119952d4d58aa5f36e5e9a7b7d2e6e src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 46290: adding do-not-demote-health-check-to-role flag to not demote health check user:group to role
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- (Updated April 15, 2016, 11:46 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description --- adding do-not-demote-health-check-to-role flag to not demote health check user:group to role Diffs (updated) - RELEASE-NOTES.md 601d2859c3ca51b30fd15de56f75bd5255a51805 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Review Request 46290: adding do-not-demote-health-check-to-role flag to not demote health check user:group to role
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46290/ --- Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-1666 https://issues.apache.org/jira/browse/AURORA-1666 Repository: aurora Description --- adding do-not-demote-health-check-to-role flag to not demote health check user:group to role Diffs - RELEASE-NOTES.md 601d2859c3ca51b30fd15de56f75bd5255a51805 src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 40a0cd6396a78c591debf5e2be11363ecf496231 src/main/python/apache/aurora/executor/common/health_checker.py d8675beb8f16ebdd2d6946367784411fe84a5cfc src/test/python/apache/aurora/executor/common/test_health_checker.py 4ab7a2fab651abd5ab8a0f312d10c444800e8b7a Diff: https://reviews.apache.org/r/46290/diff/ Testing --- - end to end tests - added unit tests Thanks, Dmitriy Shirchenko
Re: Review Request 45506: Execute shell-based health checks as the task user.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45506/#review126443 --- Ship it! Ship It! - Dmitriy Shirchenko On March 31, 2016, 8:18 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45506/ > --- > > (Updated March 31, 2016, 8:18 p.m.) > > > Review request for Aurora, Dmitriy Shirchenko and Zameer Manji. > > > Bugs: AURORA-1641 > https://issues.apache.org/jira/browse/AURORA-1641 > > > Repository: aurora > > > Description > --- > > Here's a stab at this using `os` and `pwd` modules directly to demote health > checks to the target user. > > > Diffs > - > > src/main/python/apache/aurora/common/health_check/shell.py > 6cb7dfc164f4e16143fc974d50c19a5887d32015 > src/main/python/apache/aurora/executor/common/health_checker.py > 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a > src/test/python/apache/aurora/common/health_check/test_shell.py > 7026af8c4671a40f4b517ecf12149eac34a552c8 > src/test/python/apache/aurora/executor/common/test_health_checker.py > 19c4f76347e34374c29974c182d1f4c118bcb18d > src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora > c709b035a54822ff700136963e2f942f30f38898 > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > e1c12bbd4382c31e576439f6693d82d5661029b9 > > Diff: https://reviews.apache.org/r/45506/diff/ > > > Testing > --- > > Altered an end-to-end test case to verify that shell checks are not run as a > privileged user. > > > Thanks, > > Bill Farner > >
Re: Review Request 45506: Execute shell-based health checks as the task user.
> On March 31, 2016, 4:20 p.m., Dmitriy Shirchenko wrote: > > src/main/python/apache/aurora/common/health_check/shell.py, line 66 > > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line66> > > > > why are you doing a separate try/except with timeout when > > subprocess.Popen already supports it. > > > > you can still do a kill if that's your intention in one swoop. > > Bill Farner wrote: > I'm generally a fan of scoping exceptions to catch only where they are > known to be thrown. If `Popen.__init__` and `Popen.wait` threw the same > exception type, i would probably put them in the same `try`, but in this case > they are distinct. > > However, i'm not sure what you mean by 'Popen already supports it', so > it's possible i'm missing something. > > Dmitriy Shirchenko wrote: > If you look at subprocess32, timeout is an option into Popen. What's the > logic behind changing how timeout is done via process.wait instead of relying > on Popen()? > > Bill Farner wrote: > I don't see a `timeout` kwarg on any `Popen` docs or sources. > > https://docs.python.org/3.3/library/subprocess.html#popen-constructor > > https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L716-L722 `check_call` has it. I was wrong: made a wrong assumption that Popen has it. https://github.com/google/python-subprocess32/blob/master/subprocess32.py#L587 I guess question should be rephrased: why switch to Popen when `check_call` does what we need and it just a wrapper around Popen with timeout. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45506/#review126326 --- On March 31, 2016, 6:57 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45506/ > ----------- > > (Updated March 31, 2016, 6:57 p.m.) > > > Review request for Aurora, Dmitriy Shirchenko and Zameer Manji. > > > Bugs: AURORA-1641 > https://issues.apache.org/jira/browse/AURORA-1641 > > > Repository: aurora > > > Description > --- > > Here's a stab at this using `os` and `pwd` modules directly to demote health > checks to the target user. > > > Diffs > - > > src/main/python/apache/aurora/common/health_check/shell.py > 6cb7dfc164f4e16143fc974d50c19a5887d32015 > src/main/python/apache/aurora/executor/common/health_checker.py > 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a > src/test/python/apache/aurora/common/health_check/test_shell.py > 7026af8c4671a40f4b517ecf12149eac34a552c8 > src/test/python/apache/aurora/executor/common/test_health_checker.py > 19c4f76347e34374c29974c182d1f4c118bcb18d > src/test/sh/org/apache/aurora/e2e/http/http_example_bad_healthcheck.aurora > c709b035a54822ff700136963e2f942f30f38898 > src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh > e1c12bbd4382c31e576439f6693d82d5661029b9 > > Diff: https://reviews.apache.org/r/45506/diff/ > > > Testing > --- > > Altered an end-to-end test case to verify that shell checks are not run as a > privileged user. > > > Thanks, > > Bill Farner > >
Re: Review Request 45506: Execute shell-based health checks as the task user.
> On March 31, 2016, 6:11 p.m., Zameer Manji wrote: > > The change and the tests LGTM. > > > > I currently have great ideas on how to ensure end to end validation. The > > best idea that I can provide is make use of the shell checker in the e2e > > tests. The program executed by the shell checker should just return 1 if it > > is executed as root and return 0 if it isn't. The e2e test can check for > > task failure and infer that the command was run as root if the task fails. > > Joshua Cohen wrote: > If we want something that would give us more certainty that the e2e test > behaved as expected, we could touch a file in /tmp as root (from the test > runner) and configure a shell health checker that tries to remove it. Then we > can assert that the health check failed and that the file still exists (thus > giving us confidence that the reason for the failure was permission-based and > not due to some other factor). > > Bill Farner wrote: > I was thinking something along the lines of access as well. How about a > check that tries to do something pseudo-malicious like delete `/etc/passwd`? > > Zameer Manji wrote: > +1 deleting /etc/passwd or similar it a good test. Yea, e2e has a test which makes sure a failed health check rolls back the update: https://github.com/apache/aurora/blob/master/src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh#L206 Should be super easy to modify it to roll back on a file you aren't supposed to delete. - Dmitriy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45506/#review126378 --- On March 31, 2016, 6:38 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45506/ > ----------- > > (Updated March 31, 2016, 6:38 p.m.) > > > Review request for Aurora, Dmitriy Shirchenko and Zameer Manji. > > > Bugs: AURORA-1641 > https://issues.apache.org/jira/browse/AURORA-1641 > > > Repository: aurora > > > Description > --- > > Here's a stab at this using `os` and `pwd` modules directly to demote health > checks to the target user. > > > Diffs > - > > src/main/python/apache/aurora/common/health_check/shell.py > 6cb7dfc164f4e16143fc974d50c19a5887d32015 > src/main/python/apache/aurora/executor/common/health_checker.py > 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a > src/test/python/apache/aurora/common/health_check/test_shell.py > 7026af8c4671a40f4b517ecf12149eac34a552c8 > src/test/python/apache/aurora/executor/common/test_health_checker.py > 19c4f76347e34374c29974c182d1f4c118bcb18d > > Diff: https://reviews.apache.org/r/45506/diff/ > > > Testing > --- > > I haven't spent any time thinking of a test strategy for this, but i don't > think we should proceed without end-to-end validation. I'm open to ideas > here. > > > Thanks, > > Bill Farner > >
Re: Review Request 45506: Execute shell-based health checks as the task user.
> On March 31, 2016, 4:20 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 60
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line60>
> >
> > why did you get rid of .format? i personally find it much clearer to
> > understand so would you please justify this decision.
> >
> > some refs:
> > http://stackoverflow.com/a/5082482
> > talk of deprecating % altogether in python3 and explicit mention for
> > using .format.
> > https://docs.python.org/3/whatsnew/2.6.html#pep-3101
>
> Bill Farner wrote:
> That may be true, but `%` is still the predominant pattern in this
> codebase for string formatting.
> ```console
> $ grep -R '.format(' src/{main,test}/python | wc -l
>3
> $ grep -R ' % ' src/{main,test}/python | wc -l
> 785
> ```
> I tend to follow the convention until a mass switch takes place. Patches
> welcome :-)
I see your point. My point is that objectively .format is the correct way
(given refs ^). I don't feel strongly about it, but I will say that I don't
agree with logic of going down the incorrect path as it will make it difficult
(or impossible) to correct the behavior down the line. In all of my patches, I
use .format but it looks like I am getting overruled and using .format is
discouraged :). It's not a big deal!
> On March 31, 2016, 4:20 p.m., Dmitriy Shirchenko wrote:
> > src/main/python/apache/aurora/common/health_check/shell.py, line 66
> > <https://reviews.apache.org/r/45506/diff/2/?file=1320188#file1320188line66>
> >
> > why are you doing a separate try/except with timeout when
> > subprocess.Popen already supports it.
> >
> > you can still do a kill if that's your intention in one swoop.
>
> Bill Farner wrote:
> I'm generally a fan of scoping exceptions to catch only where they are
> known to be thrown. If `Popen.__init__` and `Popen.wait` threw the same
> exception type, i would probably put them in the same `try`, but in this case
> they are distinct.
>
> However, i'm not sure what you mean by 'Popen already supports it', so
> it's possible i'm missing something.
If you look at subprocess32, timeout is an option into Popen. What's the logic
behind changing how timeout is done via process.wait instead of relying on
Popen()?
- Dmitriy
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45506/#review126326
---
On March 31, 2016, 6:38 p.m., Bill Farner wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45506/
> ---
>
> (Updated March 31, 2016, 6:38 p.m.)
>
>
> Review request for Aurora, Dmitriy Shirchenko and Zameer Manji.
>
>
> Bugs: AURORA-1641
> https://issues.apache.org/jira/browse/AURORA-1641
>
>
> Repository: aurora
>
>
> Description
> ---
>
> Here's a stab at this using `os` and `pwd` modules directly to demote health
> checks to the target user.
>
>
> Diffs
> -
>
> src/main/python/apache/aurora/common/health_check/shell.py
> 6cb7dfc164f4e16143fc974d50c19a5887d32015
> src/main/python/apache/aurora/executor/common/health_checker.py
> 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a
> src/test/python/apache/aurora/common/health_check/test_shell.py
> 7026af8c4671a40f4b517ecf12149eac34a552c8
> src/test/python/apache/aurora/executor/common/test_health_checker.py
> 19c4f76347e34374c29974c182d1f4c118bcb18d
>
> Diff: https://reviews.apache.org/r/45506/diff/
>
>
> Testing
> ---
>
> I haven't spent any time thinking of a test strategy for this, but i don't
> think we should proceed without end-to-end validation. I'm open to ideas
> here.
>
>
> Thanks,
>
> Bill Farner
>
>
Re: Review Request 45506: Execute shell-based health checks as the task user.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45506/#review126326 --- src/main/python/apache/aurora/common/health_check/shell.py (line 34) <https://reviews.apache.org/r/45506/#comment189279> document inside docstring, plz. src/main/python/apache/aurora/common/health_check/shell.py (line 56) <https://reviews.apache.org/r/45506/#comment189291> why did you get rid of .format? i personally find it much clearer to understand so would you please justify this decision. some refs: http://stackoverflow.com/a/5082482 talk of deprecating % altogether in python3 and explicit mention for using .format. https://docs.python.org/3/whatsnew/2.6.html#pep-3101 src/main/python/apache/aurora/common/health_check/shell.py (line 61) <https://reviews.apache.org/r/45506/#comment189293> why are you doing a separate try/except with timeout when subprocess.Popen already supports it. you can still do a kill if that's your intention in one swoop. src/main/python/apache/aurora/common/health_check/shell.py (line 69) <https://reviews.apache.org/r/45506/#comment189295> :nit .format instead of interpolation. src/test/python/apache/aurora/common/health_check/test_shell.py (line 24) <https://reviews.apache.org/r/45506/#comment189277> damn, my bad. thanks for catching this! - Dmitriy Shirchenko On March 30, 2016, 11:41 p.m., Bill Farner wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45506/ > --- > > (Updated March 30, 2016, 11:41 p.m.) > > > Review request for Aurora, Dmitriy Shirchenko and Zameer Manji. > > > Bugs: AURORA-1641 > https://issues.apache.org/jira/browse/AURORA-1641 > > > Repository: aurora > > > Description > --- > > Here's a stab at this using `os` and `pwd` modules directly to demote health > checks to the target user. > > > Diffs > - > > src/main/python/apache/aurora/common/health_check/shell.py > 6cb7dfc164f4e16143fc974d50c19a5887d32015 > src/main/python/apache/aurora/executor/common/health_checker.py > 28fd3ec3ef7d0b66621c0295804af0eb72c64b4a > src/test/python/apache/aurora/common/health_check/test_shell.py > 7026af8c4671a40f4b517ecf12149eac34a552c8 > src/test/python/apache/aurora/executor/common/test_health_checker.py > 19c4f76347e34374c29974c182d1f4c118bcb18d > > Diff: https://reviews.apache.org/r/45506/diff/ > > > Testing > --- > > I haven't spent any time thinking of a test strategy for this, but i don't > think we should proceed without end-to-end validation. I'm open to ideas > here. > > > Thanks, > > Bill Farner > >
Re: Review Request 44827: Do not split the shell command string passed into shell health check script
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44827/ --- (Updated March 15, 2016, 3:49 a.m.) Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji. Bugs: AURORA-1633 https://issues.apache.org/jira/browse/AURORA-1633 Repository: aurora Description --- Fixing bug where you could not pass in shell command into health checker with environment variables. When environment variable assignement was passed in, only that part would get executed and 0 would always get returned. Diffs - src/main/python/apache/aurora/common/health_check/shell.py bf63d936b3044dfa97a787938b643a52497f2d79 src/test/python/apache/aurora/common/health_check/test_shell.py 8d3a3e4e259e1ff699854aeb2434ac21f38e49ea Diff: https://reviews.apache.org/r/44827/diff/ Testing --- Unit tests + end_to_end test. Thanks, Dmitriy Shirchenko
Re: Review Request 44486: Exposing DSL defined variables to shell health checkers
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44486/ --- (Updated March 10, 2016, 6:59 p.m.) Review request for Aurora, John Sirois, Bill Farner, and Zameer Manji. Bugs: AURORA-1622 https://issues.apache.org/jira/browse/AURORA-1622 Repository: aurora Description --- Exposing DSL defined variables to shell health checkers Diffs (updated) - src/main/python/apache/aurora/common/health_check/shell.py 890bf0c5d50d0022c044a37191a2e3145cc6340f src/main/python/apache/aurora/executor/common/health_checker.py 303972778baa04e9d7dd47fb208fe1427e779976 src/test/python/apache/aurora/common/health_check/test_shell.py 84f717fbf724c11863b4980fd2740dc23fe1404e src/test/python/apache/aurora/executor/common/test_health_checker.py 9bebce8f5a26662f58075d7ce881a8bdacb2fe46 Diff: https://reviews.apache.org/r/44486/diff/ Testing --- Unit and end to end test. Thanks, Dmitriy Shirchenko
