Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Bill Farner 

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

Ship it!


Ship It!

- Bill Farner


On Aug. 14, 2014, 6:26 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 6:26 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
>   src/test/python/apache/aurora/client/api/test_updater_util.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Mark Chu-Carroll 

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

Ship it!


Ship It!

- Mark Chu-Carroll


On Aug. 14, 2014, 2:26 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 2:26 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
>   src/test/python/apache/aurora/client/api/test_updater_util.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Maxim Khutornenko 


> On Aug. 14, 2014, 5:24 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/updater_util.py, line 68
> > 
> >
> > The doc here might also help explain the behavior.  This wording would 
> > be more obvious: "Groups instance IDs into a set of contiguous integer 
> > ranges."

Done.


- Maxim


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


On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 4:08 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Maxim Khutornenko 

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

(Updated Aug. 14, 2014, 6:26 p.m.)


Review request for Aurora, Mark Chu-Carroll and Bill Farner.


Changes
---

CR comments.


Bugs: AURORA-615
https://issues.apache.org/jira/browse/AURORA-615


Repository: aurora


Description
---

Implementing client job lock and start update APIs.


Diffs (updated)
-

  src/main/python/apache/aurora/client/api/__init__.py 
371137e469fd65935f4048eaafdcf9b702e8c947 
  src/main/python/apache/aurora/client/api/updater_util.py 
cbb93e4d64a334dba9d091b68cf6d3594930fa31 
  src/test/python/apache/aurora/client/api/BUILD 
db5c22375378566f58ade5d6a0b7fcce75ad8394 
  src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
  src/test/python/apache/aurora/client/api/test_updater_util.py PRE-CREATION 

Diff: https://reviews.apache.org/r/24702/diff/


Testing
---

./pants src/test/python/apache/aurora/client/api:api


Thanks,

Maxim Khutornenko

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Maxim Khutornenko 


> On Aug. 14, 2014, 5:21 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 158
> > 
> >
> > previously-acquired

Changed.


- Maxim


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


On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 4:08 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Maxim Khutornenko 


> On Aug. 14, 2014, 5:09 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/updater_util.py, line 67
> > 
> >
> > This is a bit magical for me. It's relying on subtleties of methods 
> > that a typical python programmer will need to look up, which means that 
> > it's going to be really easy for it to be wrong in a hard-to-recognize way. 
> > 
> > Can you move it up, so that it's a classmethod, and then add a couple 
> > of tests? Tests will both check that it works, and provide demonstrations 
> > for readers to see how it works. (I know you've already got some tests of 
> > this, but they're indirect, htrough the surrounding method; for clarity, it 
> > would be better to be able to test get_ranges directly.)

Moved into a classmethod, added comments and moved unit tests into a separate 
file.


> On Aug. 14, 2014, 5:09 p.m., Mark Chu-Carroll wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 179
> > 
> >
> > Can you add a more specific error type - maybe InvalidConfigError? That 
> > way, in the client, I can generate better error messages.

Sure, done.


- Maxim


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


On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 4:08 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Maxim Khutornenko 


> On Aug. 14, 2014, 5:21 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 181
> > 
> >
> > Seems like the scheduler should automatically do this within 
> > startJobUpdate.  Is there anything preventing that behavior?

That's actually a great idea! The startJobUpdate already does 
ConfigurationManager.populateAndValidate() on the TaskConfig making this call 
effectively redundant. Will drop it entirely.


- Maxim


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


On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 4:08 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Bill Farner 

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



src/main/python/apache/aurora/client/api/updater_util.py


The doc here might also help explain the behavior.  This wording would be 
more obvious: "Groups instance IDs into a set of contiguous integer ranges."


- Bill Farner


On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 4:08 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Bill Farner 

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


LGTM, would like to reconcile the populateJobConfig situation before proceeding.


src/main/python/apache/aurora/client/api/__init__.py


previously-acquired



src/main/python/apache/aurora/client/api/__init__.py


Seems like the scheduler should automatically do this within 
startJobUpdate.  Is there anything preventing that behavior?


- Bill Farner


On Aug. 14, 2014, 4:08 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 4:08 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Re: Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Mark Chu-Carroll 

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



src/main/python/apache/aurora/client/api/__init__.py


Can you add a more specific error type - maybe InvalidConfigError? That 
way, in the client, I can generate better error messages.



src/main/python/apache/aurora/client/api/updater_util.py


This is a bit magical for me. It's relying on subtleties of methods that a 
typical python programmer will need to look up, which means that it's going to 
be really easy for it to be wrong in a hard-to-recognize way. 

Can you move it up, so that it's a classmethod, and then add a couple of 
tests? Tests will both check that it works, and provide demonstrations for 
readers to see how it works. (I know you've already got some tests of this, but 
they're indirect, htrough the surrounding method; for clarity, it would be 
better to be able to test get_ranges directly.)


- Mark Chu-Carroll


On Aug. 14, 2014, 12:08 p.m., Maxim Khutornenko wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/24702/
> ---
> 
> (Updated Aug. 14, 2014, 12:08 p.m.)
> 
> 
> Review request for Aurora, Mark Chu-Carroll and Bill Farner.
> 
> 
> Bugs: AURORA-615
> https://issues.apache.org/jira/browse/AURORA-615
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Implementing client job lock and start update APIs.
> 
> 
> Diffs
> -
> 
>   src/main/python/apache/aurora/client/api/__init__.py 
> 371137e469fd65935f4048eaafdcf9b702e8c947 
>   src/main/python/apache/aurora/client/api/updater_util.py 
> cbb93e4d64a334dba9d091b68cf6d3594930fa31 
>   src/test/python/apache/aurora/client/api/BUILD 
> db5c22375378566f58ade5d6a0b7fcce75ad8394 
>   src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/24702/diff/
> 
> 
> Testing
> ---
> 
> ./pants src/test/python/apache/aurora/client/api:api
> 
> 
> Thanks,
> 
> Maxim Khutornenko
> 
>

Review Request 24702: Implementing client job lock and start update APIs.

2014-08-14 Thread Maxim Khutornenko 

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

Review request for Aurora, Mark Chu-Carroll and Bill Farner.


Bugs: AURORA-615
https://issues.apache.org/jira/browse/AURORA-615


Repository: aurora


Description
---

Implementing client job lock and start update APIs.


Diffs
-

  src/main/python/apache/aurora/client/api/__init__.py 
371137e469fd65935f4048eaafdcf9b702e8c947 
  src/main/python/apache/aurora/client/api/updater_util.py 
cbb93e4d64a334dba9d091b68cf6d3594930fa31 
  src/test/python/apache/aurora/client/api/BUILD 
db5c22375378566f58ade5d6a0b7fcce75ad8394 
  src/test/python/apache/aurora/client/api/test_api.py PRE-CREATION 

Diff: https://reviews.apache.org/r/24702/diff/


Testing
---

./pants src/test/python/apache/aurora/client/api:api


Thanks,

Maxim Khutornenko