----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/21383/#review43388 -----------------------------------------------------------
src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java <https://reviews.apache.org/r/21383/#comment77474> Kevin and i created new procedure just last week to keep track of feature deprecations. Summary: - Create a ticket under epic AURORA-423 - LOG.warning when deprecated behavior is observed - Add a self-assigned TODO linking to your deprecation ticket src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java <https://reviews.apache.org/r/21383/#comment77470> empty line above. idea is that the line padding helps the method signature stand out more. public void longMethodSignature(..) throws SomeExceptionType { // Body starts here. } src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java <https://reviews.apache.org/r/21383/#comment77475> TODO: merge CronJobManager createJob/updateJob src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java <https://reviews.apache.org/r/21383/#comment77484> Please add test coverage for these new methods. src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java <https://reviews.apache.org/r/21383/#comment77471> ditto - empty line above src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java <https://reviews.apache.org/r/21383/#comment77476> indenting is off, here and in the catch block src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java <https://reviews.apache.org/r/21383/#comment77477> This should have a Lock guard as well. src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java <https://reviews.apache.org/r/21383/#comment77478> revert src/main/thrift/org/apache/aurora/gen/api.thrift <https://reviews.apache.org/r/21383/#comment77480> > The request will be denied if the job is already present in the schedule Is this true? src/main/thrift/org/apache/aurora/gen/api.thrift <https://reviews.apache.org/r/21383/#comment77481> Wording nit on the second sentence. How about: "The request will fail if the job has not been previously scheduled through scheduleCronJob." src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java <https://reviews.apache.org/r/21383/#comment77482> How about a test for deschedule failing? src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java <https://reviews.apache.org/r/21383/#comment77479> remove extra newline - Bill Farner On May 19, 2014, 3:25 p.m., Mark Chu-Carroll wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/21383/ > ----------------------------------------------------------- > > (Updated May 19, 2014, 3:25 p.m.) > > > Review request for Aurora, David McLaughlin and Bill Farner. > > > Bugs: aurora-417 > https://issues.apache.org/jira/browse/aurora-417 > > > Repository: aurora > > > Description > ------- > > Add cron schedule and deschedule calls to the scheduler API. > > > Diffs > ----- > > src/main/java/org/apache/aurora/scheduler/state/SchedulerCore.java > 226b71ce7749492abd3e1d673382668c9011a1c8 > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java > d377974da4f7efa7f47e06702654bc786299e01c > > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java > 4386a86c0aebdb9666c0232672f8c1bab7458f47 > src/main/thrift/org/apache/aurora/gen/api.thrift > 66292dc0369941fc62719d49209edc07adc81a53 > > src/test/java/org/apache/aurora/scheduler/state/BaseSchedulerCoreImplTest.java > 0e17f49e68953b5f043301dbb1b46b96968e1247 > src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java > 933a56bf3f7165fa84aedcc8d1392e32824fd487 > src/test/python/apache/aurora/client/api/test_scheduler_client.py > 399d4c614dda93ae0e7d8f38a02379bc0f42e4af > > Diff: https://reviews.apache.org/r/21383/diff/ > > > Testing > ------- > > Ran all tests with gradlew; no errors. (Includes two new tests, that ensure > that calling the new APIs causes the right internal cron methods to be > called.) > > > Thanks, > > Mark Chu-Carroll > >
