> On Sept. 24, 2014, 4:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java, 
> > line 195
> > <https://reviews.apache.org/r/25969/diff/1/?file=703687#file703687line195>
> >
> >     s/_get_/_fetch_ to match method name.

Fixed.


> On Sept. 24, 2014, 4:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  lines 1405-1410
> > <https://reviews.apache.org/r/25969/diff/1/?file=703689#file703689line1405>
> >
> >     How about moving this into a static helper in JobDiff for better 
> > readability?

Sure.


> On Sept. 24, 2014, 4:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java,
> >  line 1415
> > <https://reviews.apache.org/r/25969/diff/1/?file=703689#file703689line1415>
> >
> >     Suggest a more descriptive message here mentioning 
> > updateOnlyTheseInstances property name.

Done.


> On Sept. 24, 2014, 4:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java,
> >  line 148
> > <https://reviews.apache.org/r/25969/diff/1/?file=703692#file703692line148>
> >
> >     Does this translate into INVALID_REQUEST in thrift? Not sure it's a 
> > good idea unless we expose a jobDiff RPC to reliably compare job configs 
> > before the update. Otherwise, it's client diff vs. server diff with a 
> > chance of mismatch and subpar client experience.
> >     
> >     Should it rather be special-cased to return OK with a detail message 
> > stating a noop update?

Sounds good, changed to no-op/OK.


> On Sept. 24, 2014, 4:58 p.m., Maxim Khutornenko wrote:
> > src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java,
> >  line 468
> > <https://reviews.apache.org/r/25969/diff/1/?file=703692#file703692line468>
> >
> >     Unless I am reading it wrong, I am not sure how this could even be 
> > possible now that noop instances are rejected by the Diff. Is there still a 
> > case for it?

Changing from rolling forward to rolling back will be the most common.  The 
updater will flip the instances, and skip untouched instances while walking 
backwards.
It can also happen if the scheduler fails over - in-flight instances will be 
re-evaluated, and may no-op at that point.


> On Sept. 24, 2014, 4:58 p.m., Maxim Khutornenko wrote:
> > src/main/thrift/org/apache/aurora/gen/api.thrift, line 694
> > <https://reviews.apache.org/r/25969/diff/1/?file=703697#file703697line694>
> >
> >     s/../.

Fixed.


> On Sept. 24, 2014, 4:58 p.m., Maxim Khutornenko wrote:
> > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml,
> >  line 421
> > <https://reviews.apache.org/r/25969/diff/1/?file=703696#file703696line421>
> >
> >     Why left join here? Events without update ID would not make any sense.

Thanks!  I clearly can't be trusted when it comes to SQL.


- Bill


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


On Sept. 24, 2014, 12:44 a.m., Bill Farner wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25969/
> -----------------------------------------------------------
> 
> (Updated Sept. 24, 2014, 12:44 a.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
> 
> 
> Bugs: AURORA-718
>     https://issues.apache.org/jira/browse/AURORA-718
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> When creating an update, store only the delta between the initial and desired 
> states.
> 
> 
> Diffs
> -----
> 
>   src/main/java/org/apache/aurora/scheduler/base/Numbers.java 
> 5fd0814e14eee7eb827b3a3b1e6dc28ce910b7e8 
>   src/main/java/org/apache/aurora/scheduler/base/Tasks.java 
> 9664c3b0f04087ceb0451a4cc5a0b4dd6cdcee9d 
>   src/main/java/org/apache/aurora/scheduler/http/TransformationUtils.java 
> b7143941feb4824aee609741e8e2c0e015eecac3 
>   src/main/java/org/apache/aurora/scheduler/storage/ForwardingStore.java 
> 1874a3e3f9bc47104b7597fd88d77c0c36620f7d 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> cb74d62878af1efe9d58e3d4c3e5f7a84edc8de6 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStore.java 
> 15a9e2b9acd65f6268e70fb402414f5c01c42409 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
>  26cc85dcb6dccd913e9a130a134fbf8df10cc08d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  ae0320b44b55a3630e255484ea7a881daab6a7bc 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java PRE-CREATION 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> 8f9e6d6fa00840d8ff94eb7cf1c4415eb5cc1269 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  348bdbfbb51800d31b67ea1b3f2632975e598306 
>   src/main/java/org/apache/aurora/scheduler/updater/OneWayJobUpdater.java 
> 1d0bbfef583708bfdab30802724273e4b21ecf8f 
>   src/main/java/org/apache/aurora/scheduler/updater/UpdateFactory.java 
> 842256a16fe2775af865ed2ee91fd719b3476577 
>   src/main/python/apache/aurora/client/api/__init__.py 
> bbc8cf0314388b5b7cfe05562e8f9391fd722ac1 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
>  e1179b449c05adc56206bfef271a9d440a77e042 
>   src/main/thrift/org/apache/aurora/gen/api.thrift 
> 2376a5e530b12fbbebb4cfc7555656ae07795518 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DBJobUpdateStoreTest.java
>  553ef4d88fafff894a11bc6fb6d5ab33d5f6646d 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  21e8a8ba5c47f549133ea339899bf0e5097ccbb9 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> PRE-CREATION 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> f7c7205d80edac714a12c2aff0cc7db27b1909b4 
>   src/test/java/org/apache/aurora/scheduler/updater/OneWayJobUpdaterTest.java 
> ad4040c4a58d3e6f075a60712144aa6968270a55 
>   
> src/test/java/org/apache/aurora/scheduler/updater/UpdateFactoryImplTest.java 
> 548d31580b5f2bae92900fddb80005f7d274a155 
> 
> Diff: https://reviews.apache.org/r/25969/diff/
> 
> 
> Testing
> -------
> 
> ./gradlew build -Pq
> 
> 
> Thanks,
> 
> Bill Farner
> 
>

Reply via email to