aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joshua Cohen <jco...@apache.org>
Subject Re: Review Request 50168: Add rollback functionality to the scheduler
Date Thu, 21 Jul 2016 16:05:53 GMT

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


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