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




src/main/java/org/apache/aurora/scheduler/updater/UpdateInprogressException.java
 (line 22)
<https://reviews.apache.org/r/51384/#comment213570>

    s/inprogress/inProgress
    
    here and below.



src/main/python/apache/aurora/client/cli/options.py (line 89)
<https://reviews.apache.org/r/51384/#comment213579>

    You're going to want to pass the optional `maxsplit=1` arg here to handle 
the case where someone sets metadata to something like: foo=bar=baz.
    
    That said, I'm not convinced we actually need to expose this as a 
user-visible option. For the time being I think it's fine for the client to 
just generate the update id and send it without user intervention.
    
    If/when a use case for user configurable metadata arises it'll be easy 
enough to add that support then.



src/main/python/apache/aurora/client/cli/options.py (line 232)
<https://reviews.apache.org/r/51384/#comment213582>

    do you think we should package-prefix this to avoid potentially conflicting 
with other metadata values in the future (i.e. 
org.apache.aurora.client.update_id)?



src/main/python/apache/aurora/client/cli/update.py (lines 218 - 226)
<https://reviews.apache.org/r/51384/#comment213585>

    I feel like this method should either be renamed or be side effect free. As 
it stands, I would not expect a call to `_generated_id_if_needed` to also 
mutate the `metadata` param. So, either just return the id without setting it 
(leaving it up to the caller) or rename to something like 
`_populate_id_if_needed`?



src/main/python/apache/aurora/client/cli/update.py (line 237)
<https://reviews.apache.org/r/51384/#comment213587>

    Try to avoid abbreviations in variable names, so maybe 
`start_update_result` instead?



src/main/python/apache/aurora/client/cli/update.py (line 239)
<https://reviews.apache.org/r/51384/#comment213588>

    s/in/in_



src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
 (line 207)
<https://reviews.apache.org/r/51384/#comment213589>

    call this prefix `jum_` for consistency with the others?



src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql (lines 312 
- 317)
<https://reviews.apache.org/r/51384/#comment213564>

    You need to create a migration for this table. See 
https://github.com/apache/aurora/blob/master/docs/development/db-migration.md 
for details.



src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java (line 313)
<https://reviews.apache.org/r/51384/#comment213591>

    s/Inprogress/InProgress
    
    throughout



src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java (line 1609)
<https://reviews.apache.org/r/51384/#comment213592>

    Should this be in a finally to ensure a failure here doesn't break the rest 
of the tests?



src/test/python/apache/aurora/client/cli/test_options.py (line 78)
<https://reviews.apache.org/r/51384/#comment213593>

    line limit is 100 characters. I'm surprised checkstyle didn't pick this up.



src/test/python/apache/aurora/client/cli/test_supdate.py (line 156)
<https://reviews.apache.org/r/51384/#comment213594>

    Looks like there's a lot of lines beyond 100 chars here as well, can you 
fix?
    
    We'll have to figure out why checkstyle isn't calling those out as 
problematic.


- Joshua Cohen


On Aug. 24, 2016, 7:41 p.m., Santhosh Kumar Shanmugham wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51384/
> -----------------------------------------------------------
> 
> (Updated Aug. 24, 2016, 7:41 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin, Joshua Cohen, and Maxim 
> Khutornenko.
> 
> 
> Bugs: AURORA-1711
>     https://issues.apache.org/jira/browse/AURORA-1711
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Allow custom metadata to be stored with the job updates. This feature
> is to help case the reconciliation on job update request on the clients.
> 
> Today when a client's requests are proxied via a middle-man,
> if the middle-man times out before the scheduler responds with success,
> the client is left the only option of retry. In the case of updates,
> since multiple updates are not allowed in parallel, the retries fail,
> with INVALID_REQUEST. Although the original update is already accepted
> by the scheduler and is in-progress.
> 
> With this feature, clients can reconcile the job updates,
> - by marking updates them with a unique id in the metadata field
> - scheduler returns the in-progress job update in its response
> - client can match the client-generated ids to reconcile its state.
> 
> 
> Diffs
> -----
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> c5765b70501c101f0535b4eed94e9948c36808f9 
>   src/main/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStore.java 
> d2673e6b328cb1e249fbe91d18e0d9e935636eaa 
>   
> src/main/java/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.java
>  a3b04949f1726f110638e4f93ef45947cdb9e7f8 
>   src/main/java/org/apache/aurora/scheduler/storage/db/views/DbJobUpdate.java 
> 78703e92c932cd5e93ff0b70f2a0b6928f6b4003 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  929d021a336c6a3438613c9340c84a1096dc9069 
>   
> src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java
>  594bb6219294dcc77d48dcad14e2a6f9caa0c534 
>   
> src/main/java/org/apache/aurora/scheduler/updater/UpdateInprogressException.java
>  PRE-CREATION 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 9149c3018ae58d405f284fcbd4076d251ccc8192 
>   src/main/python/apache/aurora/client/cli/context.py 
> f1a256a8d09d23d8d4d4ee7d264be0fe376398c4 
>   src/main/python/apache/aurora/client/cli/options.py 
> 1245ff15a69a4b4347672f7b556985521e813a00 
>   src/main/python/apache/aurora/client/cli/update.py 
> 23aaa2c1b67599420408633733e4581553f7151b 
>   src/main/python/apache/aurora/client/hooks/hooked_api.py 
> 542f165add0f1d01a782fe6d6089bff3e736fb82 
>   
> src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml
>  8563630a179cb6d1e3b0e22c730ccbfe1c9291e2 
>   src/main/resources/org/apache/aurora/scheduler/storage/db/schema.sql 
> a40830fd668aa650c22a48cbc757b45aef27e961 
>   
> src/test/java/org/apache/aurora/scheduler/storage/db/DbJobUpdateStoreTest.java
>  08530397ff75081bde6f07f9d53317b5486e0da4 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  779dc302602ae8842084807ca868a491ea99b676 
>   src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java 
> 04551f17999d742c53dfb1b36286b119b448550e 
>   src/test/python/apache/aurora/client/cli/test_options.py 
> f2aae5741611909d5a1700cb4da70dec313e00da 
>   src/test/python/apache/aurora/client/cli/test_supdate.py 
> 92fb039fb7d3e368d7c0dfa5ebdb465c7f112416 
>   src/test/python/apache/aurora/client/cli/util.py 
> aac9f9c802e4ad1f06cee8cf3f56749760b33af5 
> 
> Diff: https://reviews.apache.org/r/51384/diff/
> 
> 
> Testing
> -------
> 
> ./build-support/jenkins/build.sh
> ./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh
> ./pants test src/test/python/apache/aurora/client/cli:cli
> 
> 
> Thanks,
> 
> Santhosh Kumar Shanmugham
> 
>

Reply via email to