aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Farner" <wfar...@apache.org>
Subject Re: Review Request 21383: Add cron schedule and deschedule calls to the scheduler API.
Date Mon, 19 May 2014 22:56:47 GMT


> On May 19, 2014, 9:11 p.m., Mark Chu-Carroll wrote:
> > src/main/java/org/apache/aurora/scheduler/state/SchedulerCoreImpl.java, line 147
> > <https://reviews.apache.org/r/21383/diff/3/?file=584300#file584300line147>
> >
> >     Just to be clear: there is no deprecation here. The existing behavior will continue
to work. We will deprecate the old cron behavior eventually, but not for a while yet.
> >     
> >     Also: is there any documented version of this procedure? I haven't seen or heard
of it before, so I assume others haven't either?
> >
> 
> Bill Farner wrote:
>     Is there any reason to not deprecate now?
>     
>     We _literally_ made it up on Friday afternoon, so no - no docs yet.  We haven't even
discussed it with anyone, so i think we're in the 'see how it works' phase.
> 
> Mark Chu-Carroll wrote:
>     Yes, there's a reason not to deprecate now: I don't want this to be a sudden change
that disrupts all of our aurora users. I'd like to start by making a new alternative way of
scheduling cron jobs, tell people, and give them some deprecation period during which to transition
to the new method, before we just pull the old way out from under them.
>     
>     This change is going to have a big impact on users. I think in the long run it'll
be positive, but it's the kind of thing where if it's not done gracefully, people will justifiably
be very upset.

I think we want the same thing :-)

My proposal is not to remove the support, but to start the deprecation cycle now, and remove
the feature in 0.7.0.


> On May 19, 2014, 9:11 p.m., Mark Chu-Carroll wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
line 258
> > <https://reviews.apache.org/r/21383/diff/3/?file=584301#file584301line258>
> >
> >     There are tests of scheduleCronJob and descheduleCronJob below. Did you not
notice those, or is there a particular kind of test that you're looking for?
> >
> 
> Bill Farner wrote:
>     Those tests cover SchedulerCoreImpl, but not SchedulerThriftInterface.  We'll need
to cover the code in both.
> 
> Maxim Khutornenko wrote:
>     Bill Farner, any reason we should continue investing into SchedulerCore? I was under
assumption we were trying to get rid of it.

Thanks, that's a good point.  We've been trying to shy away from cases where SchedulerCoreImpl
is just a proxy to another class.  Maxim correctly points out that SchedulerCoreImpl is just
a front for CronJobManager, with little additional logic.  It's probably best to just push
this code back out to SchedulerThriftInterface.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21383/#review43397
-----------------------------------------------------------


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message