Re: Review Request 39057: "aurora config read" command

2015-10-13 Thread Stephan Erb 


> On Oct. 11, 2015, 10:43 p.m., Stephan Erb wrote:
> > src/main/python/apache/aurora/client/cli/config.py, line 75
> > 
> >
> > I find it a little bit confusing that this command is called `read`. 
> > Isn't the `bind` the most important step it does?
> 
> David McLaughlin wrote:
> I just saw the IRC meeting notes for this. -1 to read but also -1 to 
> bind. I think something like evaluate, parse or even dump would be better, 
> since bind is used in the psyatchio translation sense but also used to pass 
> variables to the config from the CLI.
> 
> Maxim Khutornenko wrote:
> I actually liked the simplicity of "read" but if it raises too many 
> eyebrows my second choice would be "evaluate". Parse, dump or bind feel 
> either too shallow or too implementation-biased.
> 
> Bill Farner wrote:
> How about `tojson`?

`tojson` sounds ok. It would also make it rather obvious that it is to be used 
together with `--read-json`.


- Stephan


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


On Oct. 6, 2015, 8:43 p.m., Brian Wickman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39057/
> ---
> 
> (Updated Oct. 6, 2015, 8:43 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney and Zameer Manji.
> 
> 
> Bugs: AURORA-1504
> https://issues.apache.org/jira/browse/AURORA-1504
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> 1. Moves "metadata" into Job schema and out of AuroraConfig so that metadata 
> from bindings is saved/loaded from .json output.
> 2. Adds "config read" command that accepts same format as job create/inspect 
> etc.
> 
> This makes it simpler to build sensible deployment pipelines with binding 
> helpers.  Take an .aurora or .json config, "compile" it by making all 
> bindings concrete, check into a vcs, use for diff if necessary, 
> deploy/rollback from json without losing metadata.
> 
> 
> Diffs
> -
> 
>   .gitignore 6c37128b5d02b0e52eb467be3652a37b10d7bc06 
>   src/main/python/apache/aurora/client/cli/config.py 
> 73b556266183bf17463881b87cda107d07d79d71 
>   src/main/python/apache/aurora/config/__init__.py 
> 665e2cd08d4146e652b13bb82e04680315a1eebe 
>   src/main/python/apache/aurora/config/schema/base.py 
> 398f737bed9ef02ce4a5636896d6587bce26501e 
>   src/main/python/apache/aurora/config/thrift.py 
> b40a7fdfa63eaf18d9a85f62eec5aa717a7e7d91 
>   src/test/python/apache/aurora/config/test_thrift.py 
> 1bd745964b45b30014cabbef89949ac6221f148b 
> 
> Diff: https://reviews.apache.org/r/39057/diff/
> 
> 
> Testing
> ---
> 
> # build
> mba=aurora=; ./pants binary src/main/python/apache/aurora/client:aurora
> 
> # list configs
> mba=aurora=; dist/aurora.pex config list examples/jobs/hello_world.aurora 
> jobs=[devcluster/www-data/prod/hello]
> 
> # write to disk
> mba=aurora=; dist/aurora.pex config read devcluster/www-data/prod/hello 
> examples/jobs/hello_world.aurora > /tmp/hello_world.json
> 
> # edit file to change command, ram
> mba=aurora=; joe example/jobs/hello_world.aurora
> 
> # compare
> mba=aurora=; diff <(dist/aurora.pex config read 
> devcluster/www-data/prod/hello examples/jobs/hello_world.aurora) 
> /tmp/hello_world.json 
> 
> 44c44
> < "cmdline": "\nwhile true; do\n  echo hello 
> world with twice as much ram\n  sleep 10\ndone\n  ", 
> ---
> > "cmdline": "\nwhile true; do\n  echo hello 
> world\n  sleep 10\ndone\n  ", 
> 56c56
> < "ram": 268435456
> ---
> > "ram": 134217728
> 
> # submit
> mba=aurora=; aurora update start --read-json devcluster/www-data/prod/hello 
> /tmp/hello_world.json
> 
> 
> Thanks,
> 
> Brian Wickman
> 
>

Re: Review Request 39143: Adding getJobUpdateDiff thrift API.

2015-10-13 Thread Maxim Khutornenko 


> On Oct. 9, 2015, 11:05 p.m., Aurora ReviewBot wrote:
> > Master (f630bf7) is green with this patch.
> >   ./build-support/jenkins/build.sh
> > 
> > I will refresh this build result if you post a review containing 
> > "@ReviewBot retry"

Ping Bill, David.


- Maxim


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


On Oct. 9, 2015, 10:43 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39143/
> ---
> 
> (Updated Oct. 9, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-1515
> https://issues.apache.org/jira/browse/AURORA-1515
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding the API to return update instructions. Design doc: 
> https://docs.google.com/document/d/1Fc_YhhV7fc4D9Xv6gJzpfooxbK4YWZcvzw6Bd3qVTL8
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f56d7bdc5cdbb2fc84254c41328b01c1367c8343 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> c09130bcd4deae80e475f531e5bc08dac475f0e8 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4efcd2cd81290e919be5cf1de94c5aeb0851c67e 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java 
> a17337fb2df8e6e2a2905e88ff1f1eeb03e7d406 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> adcc02b3313220c728a9025b9aec787646f71058 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  82e7330a83ff366fab81385608c62d92445f5e16 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  83f65a73fc2053b138854be13a3b6c7748f80c2b 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> d25618c19615ee097c1f2c4b6fefc70c565c649c 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> 64d45813d89d5f084aec70cd87aadb01de7f6d41 
> 
> Diff: https://reviews.apache.org/r/39143/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39143: Adding getJobUpdateDiff thrift API.

2015-10-13 Thread David McLaughlin 

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

Ship it!


Ship It!

- David McLaughlin


On Oct. 9, 2015, 10:43 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39143/
> ---
> 
> (Updated Oct. 9, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-1515
> https://issues.apache.org/jira/browse/AURORA-1515
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding the API to return update instructions. Design doc: 
> https://docs.google.com/document/d/1Fc_YhhV7fc4D9Xv6gJzpfooxbK4YWZcvzw6Bd3qVTL8
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f56d7bdc5cdbb2fc84254c41328b01c1367c8343 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> c09130bcd4deae80e475f531e5bc08dac475f0e8 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4efcd2cd81290e919be5cf1de94c5aeb0851c67e 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java 
> a17337fb2df8e6e2a2905e88ff1f1eeb03e7d406 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> adcc02b3313220c728a9025b9aec787646f71058 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  82e7330a83ff366fab81385608c62d92445f5e16 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  83f65a73fc2053b138854be13a3b6c7748f80c2b 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> d25618c19615ee097c1f2c4b6fefc70c565c649c 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> 64d45813d89d5f084aec70cd87aadb01de7f6d41 
> 
> Diff: https://reviews.apache.org/r/39143/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39143: Adding getJobUpdateDiff thrift API.

2015-10-13 Thread Bill Farner 

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

Ship it!



src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
(line 475)


"Diff is not currently supported for cron jobs."


- Bill Farner


On Oct. 9, 2015, 3:43 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39143/
> ---
> 
> (Updated Oct. 9, 2015, 3:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-1515
> https://issues.apache.org/jira/browse/AURORA-1515
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding the API to return update instructions. Design doc: 
> https://docs.google.com/document/d/1Fc_YhhV7fc4D9Xv6gJzpfooxbK4YWZcvzw6Bd3qVTL8
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f56d7bdc5cdbb2fc84254c41328b01c1367c8343 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> c09130bcd4deae80e475f531e5bc08dac475f0e8 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4efcd2cd81290e919be5cf1de94c5aeb0851c67e 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java 
> a17337fb2df8e6e2a2905e88ff1f1eeb03e7d406 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> adcc02b3313220c728a9025b9aec787646f71058 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  82e7330a83ff366fab81385608c62d92445f5e16 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  83f65a73fc2053b138854be13a3b6c7748f80c2b 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> d25618c19615ee097c1f2c4b6fefc70c565c649c 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> 64d45813d89d5f084aec70cd87aadb01de7f6d41 
> 
> Diff: https://reviews.apache.org/r/39143/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39143: Adding getJobUpdateDiff thrift API.

2015-10-13 Thread Maxim Khutornenko 


> On Oct. 13, 2015, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java,
> >  line 479
> > 
> >
> > "Diff is not currently supported for cron jobs."

Any particular reason to word it like this? My understanding is that we don't 
have any plans to support cron job updates and this is a job update-specific 
API. Am I not getting the point?


- Maxim


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


On Oct. 9, 2015, 10:43 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39143/
> ---
> 
> (Updated Oct. 9, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-1515
> https://issues.apache.org/jira/browse/AURORA-1515
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding the API to return update instructions. Design doc: 
> https://docs.google.com/document/d/1Fc_YhhV7fc4D9Xv6gJzpfooxbK4YWZcvzw6Bd3qVTL8
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f56d7bdc5cdbb2fc84254c41328b01c1367c8343 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> c09130bcd4deae80e475f531e5bc08dac475f0e8 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4efcd2cd81290e919be5cf1de94c5aeb0851c67e 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java 
> a17337fb2df8e6e2a2905e88ff1f1eeb03e7d406 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> adcc02b3313220c728a9025b9aec787646f71058 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  82e7330a83ff366fab81385608c62d92445f5e16 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  83f65a73fc2053b138854be13a3b6c7748f80c2b 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> d25618c19615ee097c1f2c4b6fefc70c565c649c 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> 64d45813d89d5f084aec70cd87aadb01de7f6d41 
> 
> Diff: https://reviews.apache.org/r/39143/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39143: Adding getJobUpdateDiff thrift API.

2015-10-13 Thread Bill Farner 


> On Oct. 13, 2015, 1:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java,
> >  line 479
> > 
> >
> > "Diff is not currently supported for cron jobs."
> 
> Maxim Khutornenko wrote:
> Any particular reason to word it like this? My understanding is that we 
> don't have any plans to support cron job updates and this is a job 
> update-specific API. Am I not getting the point?

I feel like the functionality gap to add cron job support is minimal, and it 
would be useful.  I'm not convinced this API needs to be update-specific (i 
considered pushing back on the name like i did in the design doc, but didn't 
feel it would be constructive at this point).  The argument that scoping is 
used is not all that compelling as a basis for excluding cron jobs IMHO.


- Bill


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


On Oct. 9, 2015, 3:43 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39143/
> ---
> 
> (Updated Oct. 9, 2015, 3:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-1515
> https://issues.apache.org/jira/browse/AURORA-1515
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding the API to return update instructions. Design doc: 
> https://docs.google.com/document/d/1Fc_YhhV7fc4D9Xv6gJzpfooxbK4YWZcvzw6Bd3qVTL8
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f56d7bdc5cdbb2fc84254c41328b01c1367c8343 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> c09130bcd4deae80e475f531e5bc08dac475f0e8 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4efcd2cd81290e919be5cf1de94c5aeb0851c67e 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java 
> a17337fb2df8e6e2a2905e88ff1f1eeb03e7d406 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> adcc02b3313220c728a9025b9aec787646f71058 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  82e7330a83ff366fab81385608c62d92445f5e16 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  83f65a73fc2053b138854be13a3b6c7748f80c2b 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> d25618c19615ee097c1f2c4b6fefc70c565c649c 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> 64d45813d89d5f084aec70cd87aadb01de7f6d41 
> 
> Diff: https://reviews.apache.org/r/39143/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39143: Adding getJobUpdateDiff thrift API.

2015-10-13 Thread Maxim Khutornenko 


> On Oct. 13, 2015, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java,
> >  line 479
> > 
> >
> > "Diff is not currently supported for cron jobs."
> 
> Maxim Khutornenko wrote:
> Any particular reason to word it like this? My understanding is that we 
> don't have any plans to support cron job updates and this is a job 
> update-specific API. Am I not getting the point?
> 
> Bill Farner wrote:
> I feel like the functionality gap to add cron job support is minimal, and 
> it would be useful.  I'm not convinced this API needs to be update-specific 
> (i considered pushing back on the name like i did in the design doc, but 
> didn't feel it would be constructive at this point).  The argument that 
> scoping is used is not all that compelling as a basis for excluding cron jobs 
> IMHO.

I just don't see any benefit for cron jobs here. The main point of this API is 
the instance update sequence to be attempted if an update is scheduled. The 
reason we bundle TaskConfigs with the result is just a convenience matter for 
the client config diff (if needed). If we are not going to support cron jobs in 
the startJobUpdate API I don't see any reason to expose crons in this API. Cron 
updates are always a direct replacement. As far as diff functionality goes, all 
is needed there is getJobs API call with TaskConfig diff on the client.


- Maxim


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


On Oct. 9, 2015, 10:43 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39143/
> ---
> 
> (Updated Oct. 9, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-1515
> https://issues.apache.org/jira/browse/AURORA-1515
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding the API to return update instructions. Design doc: 
> https://docs.google.com/document/d/1Fc_YhhV7fc4D9Xv6gJzpfooxbK4YWZcvzw6Bd3qVTL8
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f56d7bdc5cdbb2fc84254c41328b01c1367c8343 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> c09130bcd4deae80e475f531e5bc08dac475f0e8 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4efcd2cd81290e919be5cf1de94c5aeb0851c67e 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java 
> a17337fb2df8e6e2a2905e88ff1f1eeb03e7d406 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> adcc02b3313220c728a9025b9aec787646f71058 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  82e7330a83ff366fab81385608c62d92445f5e16 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  83f65a73fc2053b138854be13a3b6c7748f80c2b 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> d25618c19615ee097c1f2c4b6fefc70c565c649c 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> 64d45813d89d5f084aec70cd87aadb01de7f6d41 
> 
> Diff: https://reviews.apache.org/r/39143/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39143: Adding getJobUpdateDiff thrift API.

2015-10-13 Thread Bill Farner 


> On Oct. 13, 2015, 1:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java,
> >  line 479
> > 
> >
> > "Diff is not currently supported for cron jobs."
> 
> Maxim Khutornenko wrote:
> Any particular reason to word it like this? My understanding is that we 
> don't have any plans to support cron job updates and this is a job 
> update-specific API. Am I not getting the point?
> 
> Bill Farner wrote:
> I feel like the functionality gap to add cron job support is minimal, and 
> it would be useful.  I'm not convinced this API needs to be update-specific 
> (i considered pushing back on the name like i did in the design doc, but 
> didn't feel it would be constructive at this point).  The argument that 
> scoping is used is not all that compelling as a basis for excluding cron jobs 
> IMHO.
> 
> Maxim Khutornenko wrote:
> I just don't see any benefit for cron jobs here. The main point of this 
> API is the instance update sequence to be attempted if an update is 
> scheduled. The reason we bundle TaskConfigs with the result is just a 
> convenience matter for the client config diff (if needed). If we are not 
> going to support cron jobs in the startJobUpdate API I don't see any reason 
> to expose crons in this API. Cron updates are always a direct replacement. As 
> far as diff functionality goes, all is needed there is getJobs API call with 
> TaskConfig diff on the client.

Fair enough for the API, but when it comes to presentation to the user i think 
it's best to provide a uniform command to do this and the client can do the 
right thing based on the type of job.


- Bill


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


On Oct. 9, 2015, 3:43 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39143/
> ---
> 
> (Updated Oct. 9, 2015, 3:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-1515
> https://issues.apache.org/jira/browse/AURORA-1515
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding the API to return update instructions. Design doc: 
> https://docs.google.com/document/d/1Fc_YhhV7fc4D9Xv6gJzpfooxbK4YWZcvzw6Bd3qVTL8
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f56d7bdc5cdbb2fc84254c41328b01c1367c8343 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> c09130bcd4deae80e475f531e5bc08dac475f0e8 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4efcd2cd81290e919be5cf1de94c5aeb0851c67e 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java 
> a17337fb2df8e6e2a2905e88ff1f1eeb03e7d406 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> adcc02b3313220c728a9025b9aec787646f71058 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  82e7330a83ff366fab81385608c62d92445f5e16 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  83f65a73fc2053b138854be13a3b6c7748f80c2b 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> d25618c19615ee097c1f2c4b6fefc70c565c649c 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> 64d45813d89d5f084aec70cd87aadb01de7f6d41 
> 
> Diff: https://reviews.apache.org/r/39143/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 39143: Adding getJobUpdateDiff thrift API.

2015-10-13 Thread Maxim Khutornenko 


> On Oct. 13, 2015, 8:51 p.m., Bill Farner wrote:
> > src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java,
> >  line 479
> > 
> >
> > "Diff is not currently supported for cron jobs."
> 
> Maxim Khutornenko wrote:
> Any particular reason to word it like this? My understanding is that we 
> don't have any plans to support cron job updates and this is a job 
> update-specific API. Am I not getting the point?
> 
> Bill Farner wrote:
> I feel like the functionality gap to add cron job support is minimal, and 
> it would be useful.  I'm not convinced this API needs to be update-specific 
> (i considered pushing back on the name like i did in the design doc, but 
> didn't feel it would be constructive at this point).  The argument that 
> scoping is used is not all that compelling as a basis for excluding cron jobs 
> IMHO.
> 
> Maxim Khutornenko wrote:
> I just don't see any benefit for cron jobs here. The main point of this 
> API is the instance update sequence to be attempted if an update is 
> scheduled. The reason we bundle TaskConfigs with the result is just a 
> convenience matter for the client config diff (if needed). If we are not 
> going to support cron jobs in the startJobUpdate API I don't see any reason 
> to expose crons in this API. Cron updates are always a direct replacement. As 
> far as diff functionality goes, all is needed there is getJobs API call with 
> TaskConfig diff on the client.
> 
> Bill Farner wrote:
> Fair enough for the API, but when it comes to presentation to the user i 
> think it's best to provide a uniform command to do this and the client can do 
> the right thing based on the type of job.

This is that API though, right? :) There will be a separate diff for the client 
changes. I suggest we discuss these details there.


- Maxim


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


On Oct. 9, 2015, 10:43 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39143/
> ---
> 
> (Updated Oct. 9, 2015, 10:43 p.m.)
> 
> 
> Review request for Aurora, David McLaughlin and Bill Farner.
> 
> 
> Bugs: AURORA-1515
> https://issues.apache.org/jira/browse/AURORA-1515
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Adding the API to return update instructions. Design doc: 
> https://docs.google.com/document/d/1Fc_YhhV7fc4D9Xv6gJzpfooxbK4YWZcvzw6Bd3qVTL8
> 
> 
> Diffs
> -
> 
>   api/src/main/thrift/org/apache/aurora/gen/api.thrift 
> f56d7bdc5cdbb2fc84254c41328b01c1367c8343 
>   src/main/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImpl.java 
> c09130bcd4deae80e475f531e5bc08dac475f0e8 
>   
> src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java
>  4efcd2cd81290e919be5cf1de94c5aeb0851c67e 
>   src/main/java/org/apache/aurora/scheduler/updater/JobDiff.java 
> a17337fb2df8e6e2a2905e88ff1f1eeb03e7d406 
>   src/test/java/org/apache/aurora/scheduler/thrift/Fixtures.java 
> adcc02b3313220c728a9025b9aec787646f71058 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/ReadOnlySchedulerImplTest.java
>  82e7330a83ff366fab81385608c62d92445f5e16 
>   
> src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
>  83f65a73fc2053b138854be13a3b6c7748f80c2b 
>   src/test/java/org/apache/aurora/scheduler/thrift/aop/ForwardingThrift.java 
> d25618c19615ee097c1f2c4b6fefc70c565c649c 
>   src/test/java/org/apache/aurora/scheduler/updater/JobDiffTest.java 
> 64d45813d89d5f084aec70cd87aadb01de7f6d41 
> 
> Diff: https://reviews.apache.org/r/39143/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew -Pq build
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>