Mailing-List: contact reviews-help@aurora.incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: reviews@aurora.incubator.apache.org
Content-Type: multipart/alternative;
 boundary="===============3607027079332296477=="
MIME-Version: 1.0
Subject: Re: Review Request 21383: Add cron schedule and deschedule calls to
 the
 scheduler API.
From: "Bill Farner" <wfarner@apache.org>
To: "Bill Farner" <wfarner@apache.org>,
 "David McLaughlin" <david@dmclaughlin.com>
Cc: "Aurora" <reviews@aurora.incubator.apache.org>,
 "Maxim Khutornenko" <maxim@apache.org>,
 "Mark Chu-Carroll" <mchucarroll@twopensource.com>
Date: Mon, 19 May 2014 22:56:47 -0000
Message-ID: <20140519225647.20172.52242@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Bill Farner" <noreply@reviews.apache.org>
References: <20140519211140.20171.83311@reviews.apache.org>
In-Reply-To: <20140519211140.20171.83311@reviews.apache.org>
Reply-To: "Bill Farner" <wfarner@apache.org>

--===============3607027079332296477==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit


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


--===============3607027079332296477==--