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




src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
(line 136)
<https://reviews.apache.org/r/50168/#comment208787>

    Just inline this into the call in the block below.
    
    Even better, any reason not to just take the job update key as an arg 
rather than the JobUpdate?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
(line 126)
<https://reviews.apache.org/r/50168/#comment208788>

    s/complete/completed.



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
(lines 129 - 131)
<https://reviews.apache.org/r/50168/#comment208790>

    What if the update is in `ROLLED_FORWARD` and another update has already 
run and completed since this one (i.e. there's no update in `ROLLING_FORWARD`, 
but there is a more recent update in `ROLLED_FORWARD`)?



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
(line 131)
<https://reviews.apache.org/r/50168/#comment208789>

    s/no/not



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
(line 251)
<https://reviews.apache.org/r/50168/#comment208798>

    I might be missing something, but I don't see a check in this method that 
if the update is complete, it's in the `ROLLED_FORWARD` state (as described by 
the javadoc on the interface declaration).



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
(line 271)
<https://reviews.apache.org/r/50168/#comment208791>

    "No job update found for " + key



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
(lines 279 - 280)
<https://reviews.apache.org/r/50168/#comment208792>

    What is the purpose of this call if it's not using the return value? As far 
as I can tell there are no side effects from the call.



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
(line 282)
<https://reviews.apache.org/r/50168/#comment208793>

    Might be useful to call this out as a rollback update in the log (i.e. 
"Starting rollback update for job " + job).



src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 
(line 287)
<https://reviews.apache.org/r/50168/#comment208794>

    indentation is off here, this should be aligned w/ `ILockKey.build` from 
the previous line.



src/main/python/apache/aurora/client/cli/update.py (lines 106 - 111)
<https://reviews.apache.org/r/50168/#comment208801>

    This is not going to work for updates that are complete, as 
`_modify_update` fails if the current state of the update is not an in-progress 
state:
    
    
https://github.com/apache/aurora/blob/master/src/main/python/apache/aurora/client/cli/update.py#L61-L76



src/main/python/apache/aurora/client/cli/update.py (line 332)
<https://reviews.apache.org/r/50168/#comment208800>

    Maybe something like "Rollback the latest in-progress or completed update." 
reads a little bit better?


- Joshua Cohen


On July 21, 2016, 3:37 a.m., Igor Morozov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50168/
> -----------------------------------------------------------
> 
> (Updated July 21, 2016, 3:37 a.m.)
> 
> 
> Review request for Aurora, Joshua Cohen, Maxim Khutornenko, Bill Farner, and 
> Zameer Manji.
> 
> 
> Bugs: AURORA-1721
>     https://issues.apache.org/jira/browse/AURORA-1721
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Add rollback functionality to the scheduler
> 
> Thic change also includes an addition to storage replicated log. I needed a 
> way to re-insert lock token for an existing job update.
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> 1d66208490aff6ea8af4c737845fa2cf13617529 
>   api/src/main/thrift/org/apache/aurora/gen/storage.thrift 
> 9e4213f13255a182df938bea44ca87fa03a25318 
>   src/main/java/org/apache/aurora/scheduler/storage/JobUpdateStore.java 
> 52c3c6618a3cf1009435ca8a9cece36365913e55 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   src/main/java/org/apache/aurora/scheduler/storage/log/LogStorage.java 
> 39924c62108f6a68f545f90d77ceab4265d41fad 
>   
> src/main/java/org/apache/aurora/scheduler/storage/log/WriteAheadStorage.java 
> d0de063fd78e6c4f62fae4a598d1d22f9775772d 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  b534abf95bab6e1657e3ef993cf34c0d6ec460be 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/aop/AnnotatedAuroraAdmin.java
>  9243c92b11040b68ed6014b3991db69fc08bcddf 
>   src/main/java/org/apache/aurora/scheduler/updater/JobUpdateController.java 
> f8357c46df1b025bf4e38a7ce1cb1c13a50c39f9 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  364c5c753f884a2d89e27802d7bbf3b2b6d3a08e 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 68baf8fdb90cd26100159401c46c9963c24332b3 
>   src/main/python/apache/aurora/client/cli/update.py 
> bb526f7bf94d7bfe02fe2786493c85be1bfeb86f 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> e157c0dfde5efc418448e138aa008ade742fe816 
>   src/test/python/apache/aurora/client/api/test_scheduler_client.py 
> afbd385b7eda64cb1f7d118b695e65e4045eac6c 
> 
> Diff: https://reviews.apache.org/r/50168/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Igor Morozov
> 
>

Reply via email to