Re: Review Request 39764: Add note to CONTRIBUTING.md about default reviewers.

2015-10-29 Thread Bill Farner

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

Ship it!


Thanks!

- Bill Farner


On Oct. 29, 2015, 11:29 a.m., Joshua Cohen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39764/
> ---
> 
> (Updated Oct. 29, 2015, 11:29 a.m.)
> 
> 
> Review request for Aurora and Bill Farner.
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Add note to CONTRIBUTING.md about default reviewers.
> 
> 
> Diffs
> -
> 
>   CONTRIBUTING.md 9a27950f4a9ff974e9342c8022487dd9b1eccc96 
> 
> Diff: https://reviews.apache.org/r/39764/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joshua Cohen
> 
>



Re: Review Request 39670: Modify ClusterStateImpl to be thread safe.

2015-10-29 Thread Bill Farner

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

Ship it!


Ship It!

- Bill Farner


On Oct. 29, 2015, 1:44 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39670/
> ---
> 
> (Updated Oct. 29, 2015, 1:44 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1510
> https://issues.apache.org/jira/browse/AURORA-1510
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> ClusterStateImpl exposes a synchronized multimap which does not have a 
> thread-safe implementation of `keySet`. This patch revises the `ClusterState` 
> interface to offer more precise operations and modifies `ClusterStateImpl` to 
> makes all of these operations thread safe.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java 
> 42e2ca49b3c5de997cb5363e3518512d0f5a725e 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/ClusterStateImplTest.java 
> a1ac922d471013779710e02c0c9ca9f84b506807 
> 
> Diff: https://reviews.apache.org/r/39670/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> If the approach is good, I can update this with benchmark information.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>



Re: Review Request 39670: Modify ClusterStateImpl to be thread safe.

2015-10-29 Thread Zameer Manji

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

(Updated Oct. 29, 2015, 1:44 p.m.)


Review request for Aurora, Maxim Khutornenko and Bill Farner.


Changes
---

Bill's feedback.


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


Repository: aurora


Description
---

ClusterStateImpl exposes a synchronized multimap which does not have a 
thread-safe implementation of `keySet`. This patch revises the `ClusterState` 
interface to offer more precise operations and modifies `ClusterStateImpl` to 
makes all of these operations thread safe.


Diffs (updated)
-

  src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java 
42e2ca49b3c5de997cb5363e3518512d0f5a725e 
  src/test/java/org/apache/aurora/scheduler/preemptor/ClusterStateImplTest.java 
a1ac922d471013779710e02c0c9ca9f84b506807 

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


Testing
---

./gradlew build -Pq

If the approach is good, I can update this with benchmark information.


Thanks,

Zameer Manji



Re: Review Request 39670: Modify ClusterStateImpl to be thread safe.

2015-10-29 Thread Zameer Manji


> On Oct. 28, 2015, 8:02 a.m., Bill Farner wrote:
> > It's not clear to me is why you didn't decide to return an immutable 
> > multimap copy and stay behind the previous interface.  (Based on the 
> > typical characteristics of the map, and the way it's used here i don't 
> > imagine a big perf difference from this patch.)  Did you consider that?

I did consider this approach before, but I could not get the tests to pass. I 
later determined that it was due to the nature of Multimap#equals. I have 
updated the implementation to reflect this.


- Zameer


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


On Oct. 27, 2015, 9:17 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39670/
> ---
> 
> (Updated Oct. 27, 2015, 9:17 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1510
> https://issues.apache.org/jira/browse/AURORA-1510
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> ClusterStateImpl exposes a synchronized multimap which does not have a 
> thread-safe implementation of `keySet`. This patch revises the `ClusterState` 
> interface to offer more precise operations and modifies `ClusterStateImpl` to 
> makes all of these operations thread safe.
> 
> 
> Diffs
> -
> 
>   src/main/java/org/apache/aurora/scheduler/preemptor/ClusterState.java 
> ce3bc7e6da3f86625c690e26c28ccef67ed9021a 
>   src/main/java/org/apache/aurora/scheduler/preemptor/ClusterStateImpl.java 
> 42e2ca49b3c5de997cb5363e3518512d0f5a725e 
>   
> src/main/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessor.java 
> 506176769e172b7e9f4ba05c486fe6ab550fb5c3 
>   
> src/test/java/org/apache/aurora/scheduler/preemptor/PendingTaskProcessorTest.java
>  49002d03b190b8642b6251f4d1fbd6663ee6becc 
> 
> Diff: https://reviews.apache.org/r/39670/diff/
> 
> 
> Testing
> ---
> 
> ./gradlew build -Pq
> 
> If the approach is good, I can update this with benchmark information.
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>



Review Request 39784: Upgrade Aurora to pants 0.0.55.

2015-10-29 Thread John Sirois

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

Review request for Aurora, Joe Smith, Bill Farner, and Zameer Manji.


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


Repository: aurora


Description
---

This also brings Aurora up to pex 1.1.0 and switches to the pants
setup script; an equivalent to gradlew.  Of note, the script is checked
in with chmod 555, its not intended to be edited.

Although pants now includes checkstyle built in, conversion to use it is
left for follow-on work.

Also adapt make-pycharm-virtualenv which previously relied on the local
bootstrapping of pants - now hidden away by the pants setup script.

 .pantsversion  |   1 -
 3rdparty/python/requirements.txt   |   2 +-
 BUILD  |  25 -
 BUILD.tools|  19 --
 build-support/jenkins/build.sh |   2 +-
 build-support/pants_requirements.txt   |  30 ---
 build-support/python/make-pycharm-virtualenv   |  11 +-
 build-support/python/update-pants-requirements |  34 -
 pants  | 102 
+++---
 pants.ini  |  63 
++-
 src/main/python/apache/aurora/tools/BUILD  |   2 +-
 src/main/python/apache/thermos/observer/BUILD  |   2 +-
 12 files changed, 114 insertions(+), 179 deletions(-)


Diffs
-

  .pantsversion 78bae5bb6d254d014e35be0b828497f1509d80bd 
  3rdparty/python/requirements.txt 1eeb36dee1a0ebd33999bbb3327338a51cba00d7 
  BUILD 7de0c74b03ba609576867ba96885858c0908f2e9 
  BUILD.tools 75698a5ded7914a4d22ab7ae769d9ed1576531e4 
  build-support/jenkins/build.sh 5606bb157cb117a588f363382d7c8841ae957138 
  build-support/pants_requirements.txt fad8da5ce4c03f25554394bc628d4fbb0fe6cd69 
  build-support/python/make-pycharm-virtualenv 
d7bd41835bcd9a8fe62ea522a177bfa7830a897b 
  build-support/python/update-pants-requirements 
82f7c5136fad52f4bc2a458beb5f34f0cc7f6cec 
  pants 6f3526ef76fc37e3215b0673bcb1725d205a1c95 
  pants.ini dd4ba668586ee5bd1c888f315429e661adb6a480 
  src/main/python/apache/aurora/tools/BUILD 
e5ac75838cbe98bbef4e35f6f300b6a6df5e7de5 
  src/main/python/apache/thermos/observer/BUILD 
d7eedabc0930711530b45ac98a1159e69d1a0c00 

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


Testing
---

Locally: `./pants test.pytest --no-fast src/test/python:: -- -v`

Also generated a pycharm project via:
  `./build-support/python/make-pycharm-virtualenv`
Confirmed library source linking worked as did running unit tests
via the IDE.

Also grepped for pants commands in the repo, found `binary` and `setup-py`
and confirmed these worked.


Thanks,

John Sirois



Re: Review Request 39784: Upgrade Aurora to pants 0.0.55.

2015-10-29 Thread Zameer Manji

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


John, would you mind filing a ticket to track the followup work to use the 
built in pants checkstyle?

- Zameer Manji


On Oct. 29, 2015, 3:05 p.m., John Sirois wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39784/
> ---
> 
> (Updated Oct. 29, 2015, 3:05 p.m.)
> 
> 
> Review request for Aurora, Joe Smith, Bill Farner, and Zameer Manji.
> 
> 
> Bugs: AURORA-1499
> https://issues.apache.org/jira/browse/AURORA-1499
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> This also brings Aurora up to pex 1.1.0 and switches to the pants
> setup script; an equivalent to gradlew.  Of note, the script is checked
> in with chmod 555, its not intended to be edited.
> 
> Although pants now includes checkstyle built in, conversion to use it is
> left for follow-on work.
> 
> Also adapt make-pycharm-virtualenv which previously relied on the local
> bootstrapping of pants - now hidden away by the pants setup script.
> 
>  .pantsversion  |   1 -
>  3rdparty/python/requirements.txt   |   2 +-
>  BUILD  |  25 -
>  BUILD.tools|  19 --
>  build-support/jenkins/build.sh |   2 +-
>  build-support/pants_requirements.txt   |  30 ---
>  build-support/python/make-pycharm-virtualenv   |  11 +-
>  build-support/python/update-pants-requirements |  34 -
>  pants  | 102 
> +++---
>  pants.ini  |  63 
> ++-
>  src/main/python/apache/aurora/tools/BUILD  |   2 +-
>  src/main/python/apache/thermos/observer/BUILD  |   2 +-
>  12 files changed, 114 insertions(+), 179 deletions(-)
> 
> 
> Diffs
> -
> 
>   .pantsversion 78bae5bb6d254d014e35be0b828497f1509d80bd 
>   3rdparty/python/requirements.txt 1eeb36dee1a0ebd33999bbb3327338a51cba00d7 
>   BUILD 7de0c74b03ba609576867ba96885858c0908f2e9 
>   BUILD.tools 75698a5ded7914a4d22ab7ae769d9ed1576531e4 
>   build-support/jenkins/build.sh 5606bb157cb117a588f363382d7c8841ae957138 
>   build-support/pants_requirements.txt 
> fad8da5ce4c03f25554394bc628d4fbb0fe6cd69 
>   build-support/python/make-pycharm-virtualenv 
> d7bd41835bcd9a8fe62ea522a177bfa7830a897b 
>   build-support/python/update-pants-requirements 
> 82f7c5136fad52f4bc2a458beb5f34f0cc7f6cec 
>   pants 6f3526ef76fc37e3215b0673bcb1725d205a1c95 
>   pants.ini dd4ba668586ee5bd1c888f315429e661adb6a480 
>   src/main/python/apache/aurora/tools/BUILD 
> e5ac75838cbe98bbef4e35f6f300b6a6df5e7de5 
>   src/main/python/apache/thermos/observer/BUILD 
> d7eedabc0930711530b45ac98a1159e69d1a0c00 
> 
> Diff: https://reviews.apache.org/r/39784/diff/
> 
> 
> Testing
> ---
> 
> Locally: `./pants test.pytest --no-fast src/test/python:: -- -v`
> 
> Also generated a pycharm project via:
>   `./build-support/python/make-pycharm-virtualenv`
> Confirmed library source linking worked as did running unit tests
> via the IDE.
> 
> Also grepped for pants commands in the repo, found `binary` and `setup-py`
> and confirmed these worked.
> 
> 
> Thanks,
> 
> John Sirois
> 
>



Re: Review Request 39629: Remove ChainedPathDetector and root argument from the observer.

2015-10-29 Thread Aurora ReviewBot

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


Master (bcb4774) is green with this patch.
  ./build-support/jenkins/build.sh

However, it appears that it might lack test coverage.

I will refresh this build result if you post a review containing "@ReviewBot 
retry"

- Aurora ReviewBot


On Oct. 29, 2015, 10:06 p.m., Zameer Manji wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39629/
> ---
> 
> (Updated Oct. 29, 2015, 10:06 p.m.)
> 
> 
> Review request for Aurora, Maxim Khutornenko and Bill Farner.
> 
> 
> Bugs: AURORA-1338
> https://issues.apache.org/jira/browse/AURORA-1338
> 
> 
> Repository: aurora
> 
> 
> Description
> ---
> 
> Remove ChainedPathDetector and root argument from the observer.
> 
> 
> Diffs
> -
> 
>   NEWS 40fe13985a7f8d69252edac37187f1993a05c47c 
>   examples/vagrant/upstart/aurora-thermos-observer.conf 
> d7d649dc8aaacd98772183fa1429ef8fc84fa20a 
>   src/main/python/apache/aurora/tools/thermos_observer.py 
> 82de0a14b3de2bf3d1b7282fe7dc9fc185d7 
> 
> Diff: https://reviews.apache.org/r/39629/diff/
> 
> 
> Testing
> ---
> 
> e2e tests passed
> 
> 
> Thanks,
> 
> Zameer Manji
> 
>



Review Request 39797: Adding help message in case kerberos auth fails.

2015-10-29 Thread Maxim Khutornenko

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

Review request for Aurora, Joshua Cohen and Bill Farner.


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


Repository: aurora


Description
---

The only reasonable way to wire it in is via the AuthModule, which is the only 
entity aware of the auth approach used.

Also, dropped AuthModule.payload() as it's no longer needed (SessionKey thrift 
struct is empty now).


Diffs
-

  src/main/python/apache/aurora/client/api/scheduler_client.py 
8ff21bf64c4a2e6678299439af42e9a0eeacbc7e 
  src/main/python/apache/aurora/common/auth/auth_module.py 
e655cad8fbd3d23e3c44cbc38f6b6462d4781d9b 
  src/main/python/apache/aurora/common/auth/auth_module_manager.py 
2d785e7b274cb3be0b96322bc6b65a3c1ef5dfea 
  src/main/python/apache/aurora/kerberos/auth_module.py 
fade00504a3122bc4b93e1cc1b7ebac6415303da 
  src/test/python/apache/aurora/client/api/test_scheduler_client.py 
1b4ff9cef167de1e4051e6aaa6e4fb6961fd20f1 

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


Testing
---

./pants test.pytest --no-fast src/test/python::
./src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh

Example error message:
```
...
GSSError: ((' Miscellaneous failure (see text)', 851968), ('No credentials 
cache file found', -1765328189))
ERROR] Communication with Aurora scheduler is kerberized. Did you forget to run 
"kinit"?
401 Client Error: Authorization Required
```


Thanks,

Maxim Khutornenko