Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Joe Smith
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73992 --- Ship it! I can only wonder how many times this has caused an error

Re: Review Request 30710: add mesos role feature

2015-02-24 Thread lozh...@ebay.com zhang
> On Feb. 17, 2015, 11:09 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/Resources.java, > > line 174 > > > > > > Prefer using guava `Ordering` [1] instead. It will avoid ex

Re: Review Request 30710: add mesos role feature

2015-02-24 Thread lozh...@ebay.com zhang
> On Feb. 17, 2015, 11:09 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/Resources.java, > > line 114 > > > > > > This is breaking Resources/Offer encapsulation and is not a

Re: Review Request 30710: add mesos role feature

2015-02-24 Thread lozh...@ebay.com zhang
> On Feb. 17, 2015, 11:09 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/configuration/Resources.java, > > line 114 > > > > > > This is breaking Resources/Offer encapsulation and is not a

Re: Review Request 31394: Renaming storage.thrift job store structs.

2015-02-24 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31394/#review73958 --- Ship it! Ship It! - Bill Farner On Feb. 25, 2015, 1:06 a.m., Max

Re: Review Request 31394: Renaming storage.thrift job store structs.

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31394/#review73955 --- Ship it! Master (cd681d9) is green with this patch. ./build-suppo

Re: Review Request 31389: Remove LiveClusterState, make CachedClusterState (now ClusterStateImpl) default.

2015-02-24 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31389/#review73954 --- Ship it! Ship It! - Maxim Khutornenko On Feb. 25, 2015, 12:01 a.

Re: Review Request 31394: Renaming storage.thrift job store structs.

2015-02-24 Thread Kevin Sweeney
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31394/#review73953 --- Ship it! Ship It! - Kevin Sweeney On Feb. 24, 2015, 5:06 p.m., M

Review Request 31394: Renaming storage.thrift job store structs.

2015-02-24 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31394/ --- Review request for Aurora, Kevin Sweeney and Bill Farner. Repository: aurora

Re: Review Request 31389: Remove LiveClusterState, make CachedClusterState (now ClusterStateImpl) default.

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31389/#review73949 --- Ship it! Master (cd681d9) is green with this patch. ./build-suppo

Re: Review Request 31388: Update thrift API and internal code to use JobUpdateSummary.key rather than job key and id.

2015-02-24 Thread Bill Farner
> On Feb. 25, 2015, 12:17 a.m., Aurora ReviewBot wrote: > > Master (cd681d9) is red with this patch. > > ./build-support/jenkins/build.sh > > > >  role=role, > >  jobKey=job_key.to_thrift() if > > job_key else Non

Re: Review Request 31388: Update thrift API and internal code to use JobUpdateSummary.key rather than job key and id.

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31388/#review73945 --- Master (cd681d9) is red with this patch. ./build-support/jenkins/b

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73943 --- Drive-by, i realizer you're in-flight on an update to the diff so i

Re: Review Request 31388: Update thrift API and internal code to use JobUpdateSummary.key rather than job key and id.

2015-02-24 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31388/ --- (Updated Feb. 25, 2015, 12:07 a.m.) Review request for Aurora, Maxim Khutornenk

Re: Review Request 31388: Update thrift API and internal code to use JobUpdateSummary.key rather than job key and id.

2015-02-24 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31388/ --- (Updated Feb. 25, 2015, midnight) Review request for Aurora, Maxim Khutornenko

Review Request 31389: Remove LiveClusterState, make CachedClusterState (now ClusterStateImpl) default.

2015-02-24 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31389/ --- Review request for Aurora, Joshua Cohen and Maxim Khutornenko. Bugs: AURORA-105

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Kevin Sweeney
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73939 --- Ship it! src/test/python/apache/aurora/common/test_clusters.py

Re: Review Request 31380: Now that we've identified not calling .converge as a source of flakiness, remove epsilon from health checker tests.

2015-02-24 Thread Joe Smith
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31380/#review73938 --- Ship it! Ship It! - Joe Smith On Feb. 24, 2015, 1:27 p.m., Brian

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Kevin Sweeney
> On Feb. 24, 2015, 11:12 a.m., Kevin Sweeney wrote: > > This is awesome! If this fixes the full suite, can you also remove > > `--no-fast` from `build-support/jenkins/build.sh`? > > Stephan Erb wrote: > I doubt that this patch is sufficient to warrant the change of the build > script. The

Re: Review Request 31388: Update thrift API and internal code to use JobUpdateSummary.key rather than job key and id.

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31388/#review73936 --- This patch does not apply cleanly on master (cd681d9), do you need t

Review Request 31388: Update thrift API and internal code to use JobUpdateSummary.key rather than job key and id.

2015-02-24 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31388/ --- Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-109

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Steve Niemitz
> On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: > > > > Steve Niemitz wrote: > I'm not a big fan of how the parsing works here either. I was thinking > about this last night, I think I have a better plan here. Lemme know what > you think. > > I already want to add volume supp

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Stephan Erb
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/ --- (Updated Feb. 24, 2015, 11:12 p.m.) Review request for Aurora, Kevin Sweeney an

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73928 --- Ship it! Master (cd681d9) is green with this patch. ./build-suppo

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Stephan Erb
> On Feb. 24, 2015, 8:12 p.m., Kevin Sweeney wrote: > > This is awesome! If this fixes the full suite, can you also remove > > `--no-fast` from `build-support/jenkins/build.sh`? I doubt that this patch is sufficient to warrant the change of the build script. There may be many more error condit

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73925 --- Ship it! src/test/python/apache/aurora/common/test_clusters.py

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Stephan Erb
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/ --- (Updated Feb. 24, 2015, 10:52 p.m.) Review request for Aurora. Changes --

Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review73923 --- Ship it! Master (2aa90e7) is green with this patch. ./build-suppo

Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-24 Thread Maxim Khutornenko
> On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote: > > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 317 > > > > > > Same as above - no need to hold the intrinsic lock while logging here. >

Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-24 Thread Kevin Sweeney
> On Feb. 24, 2015, 11:35 a.m., Kevin Sweeney wrote: > > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 317 > > > > > > Same as above - no need to hold the intrinsic lock while logging here. >

Re: Review Request 31380: Now that we've identified not calling .converge as a source of flakiness, remove epsilon from health checker tests.

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31380/#review73920 --- Ship it! Master (2aa90e7) is green with this patch. ./build-suppo

Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-24 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/ --- (Updated Feb. 24, 2015, 9:30 p.m.) Review request for Aurora, Kevin Sweeney and

Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-24 Thread Maxim Khutornenko
> On Feb. 24, 2015, 7:35 p.m., Kevin Sweeney wrote: > > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 301 > > > > > > No need to hold the intrinsic lock while logging here Having an explicit

Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-24 Thread Maxim Khutornenko
> On Feb. 23, 2015, 10:52 p.m., Bill Farner wrote: > > src/main/java/org/apache/aurora/scheduler/async/OfferManager.java, line 255 > > > > > > s/may/will/? Changed. - Maxim

Review Request 31380: Now that we've identified not calling .converge as a source of flakiness, remove epsilon from health checker tests.

2015-02-24 Thread Brian Wickman
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31380/ --- Review request for Aurora and Joe Smith. Repository: aurora Description -

Re: Review Request 31241: Pushing transactions up in QuotaManager.

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31241/#review73916 --- Master (70f3bf4) is red with this patch. ./build-support/jenkins/b

Re: Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Bill Farner
> On Feb. 24, 2015, 8:54 p.m., Zameer Manji wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, > > line 149 > > > > > > Have you filed an upstream bug about this behaviour

Re: Review Request 31241: Pushing transactions up in QuotaManager.

2015-02-24 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31241/ --- (Updated Feb. 24, 2015, 9:04 p.m.) Review request for Aurora and Bill Farner.

Re: Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Maxim Khutornenko
> On Feb. 24, 2015, 8:54 p.m., Zameer Manji wrote: > > src/main/java/org/apache/aurora/scheduler/updater/Updates.java, line 57 > > > > > > I don't think we should pass around any mutable state. I think data > > objects

Re: Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Bill Farner
> On Feb. 24, 2015, 8:54 p.m., Zameer Manji wrote: > > src/main/resources/org/apache/aurora/scheduler/storage/db/JobUpdateDetailsMapper.xml, > > line 149 > > > > > > Have you filed an upstream bug about this behaviour

Re: Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31376/#review73906 --- src/main/java/org/apache/aurora/scheduler/updater/Updates.java

Re: Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Zameer Manji
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31376/#review73907 --- Ship it! Ship It! - Zameer Manji On Feb. 24, 2015, 12:12 p.m., B

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Bill Farner
> On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: > > > > Steve Niemitz wrote: > I'm not a big fan of how the parsing works here either. I was thinking > about this last night, I think I have a better plan here. Lemme know what > you think. > > I already want to add volume supp

Re: Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Bill Farner
> On Feb. 24, 2015, 8:28 p.m., Maxim Khutornenko wrote: > > src/main/java/org/apache/aurora/scheduler/updater/Updates.java, line 57 > > > > > > Would it make sense to accept/return `JobUpdateSummary`? The > > mutable/i

Re: Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31376/#review73900 --- Ship it! Master (19378c1) is green with this patch. ./build-suppo

Re: Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31376/#review73899 --- Ship it! src/main/java/org/apache/aurora/scheduler/updater/Updates

Review Request 31376: Introduce JobUpdateSummary.key field, dual write that field when it is read/received.

2015-02-24 Thread Bill Farner
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31376/ --- Review request for Aurora, Maxim Khutornenko and Zameer Manji. Bugs: AURORA-109

Re: Review Request 30895: Offer filtering for static vetoes. Part 4 of 4: Benchmarks.

2015-02-24 Thread Kevin Sweeney
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30895/#review73887 --- Ship it! Ship It! - Kevin Sweeney On Feb. 23, 2015, 2:47 p.m., M

Re: Review Request 31235: Refactoring CronJobManager interface.

2015-02-24 Thread Kevin Sweeney
> On Feb. 24, 2015, 11:27 a.m., Kevin Sweeney wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronLifecycle.java, > > line 63 > > > > > > This TODO wasn't addressed - put it back? > > Maxim Khutornenk

Re: Review Request 29943: Uptime-driven scheduler job updates

2015-02-24 Thread Maxim Khutornenko
> On Feb. 24, 2015, 7:30 p.m., Kevin Sweeney wrote: > > Is this ready for review now? It is. However, since AURORA-1041 is still in Open I am going to discard it and repost when the ticket moves into Accepted. - Maxim --- This is an au

Re: Review Request 31235: Refactoring CronJobManager interface.

2015-02-24 Thread Maxim Khutornenko
> On Feb. 24, 2015, 7:27 p.m., Kevin Sweeney wrote: > > src/main/java/org/apache/aurora/scheduler/cron/quartz/CronJobManagerImpl.java, > > line 142 > > > > > > rename to cronJobStore? I will address this shortly in a

Re: Review Request 30891: Offer filtering for static vetoes. Part 3 of 4: Offer filtering.

2015-02-24 Thread Kevin Sweeney
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30891/#review72448 --- Ship it! src/main/java/org/apache/aurora/scheduler/state/TaskAssig

Re: Review Request 29943: Uptime-driven scheduler job updates

2015-02-24 Thread Kevin Sweeney
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29943/#review73877 --- Is this ready for review now? - Kevin Sweeney On Jan. 20, 2015, 1

Re: Review Request 31235: Refactoring CronJobManager interface.

2015-02-24 Thread Kevin Sweeney
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31235/#review73712 --- Ship it! src/main/java/org/apache/aurora/scheduler/cron/quartz/Cro

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Kevin Sweeney
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73867 --- This is awesome! If this fixes the full suite, can you also remove `

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Steve Niemitz
> On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: > > > > Steve Niemitz wrote: > I'm not a big fan of how the parsing works here either. I was thinking > about this last night, I think I have a better plan here. Lemme know what > you think. > > I already want to add volume supp

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Joshua Cohen
> On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: > > > > Steve Niemitz wrote: > I'm not a big fan of how the parsing works here either. I was thinking > about this last night, I think I have a better plan here. Lemme know what > you think. > > I already want to add volume supp

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Bill Farner
> On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: > > > > Steve Niemitz wrote: > I'm not a big fan of how the parsing works here either. I was thinking > about this last night, I think I have a better plan here. Lemme know what > you think. > > I already want to add volume supp

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Maxim Khutornenko
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73832 --- src/test/python/apache/aurora/common/test_clusters.py

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Steve Niemitz
> On Feb. 24, 2015, 3:34 p.m., Stephan Erb wrote: > > That's interesting, I didn't know the filesystem isolator would pick up volumes and mount them as well. I think its outside the scope of this patch to refactor the code for this, however, I created AURORA-1140 to track it, as I agree it'd

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Stephan Erb
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31338/#review73822 --- src/main/java/org/apache/aurora/scheduler/mesos/MesosTaskFactory.ja

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Steve Niemitz
> On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: > > > > Steve Niemitz wrote: > I'm not a big fan of how the parsing works here either. I was thinking > about this last night, I think I have a better plan here. Lemme know what > you think. > > I already want to add volume supp

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Stephan Erb
> On Feb. 24, 2015, 7:01 a.m., Joshua Cohen wrote: > > > > Steve Niemitz wrote: > I'm not a big fan of how the parsing works here either. I was thinking > about this last night, I think I have a better plan here. Lemme know what > you think. > > I already want to add volume supp

Re: Review Request 31338: Added a command line flag, -global_container_mounts, to allow mounting paths from the slaves into the (docker) containers they run.

2015-02-24 Thread Steve Niemitz
> On Feb. 24, 2015, 6:01 a.m., Joshua Cohen wrote: > > I'm not a big fan of how the parsing works here either. I was thinking about this last night, I think I have a better plan here. Lemme know what you think. I already want to add volume support per-job at some point, so I propose adding

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73815 --- Ship it! Master (19378c1) is green with this patch. ./build-suppo

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Stephan Erb
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/ --- (Updated Feb. 24, 2015, 3:18 p.m.) Review request for Aurora. Changes ---

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73807 --- Master (19378c1) is red with this patch. ./build-support/jenkins/b

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Stephan Erb
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73806 --- @ReviewBot retry - Stephan Erb On Feb. 24, 2015, 11:57 a.m., Step

Re: Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Aurora ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/#review73792 --- Master (19378c1) is red with this patch. ./build-support/jenkins/b

Review Request 31350: Fix clusters.patch contextmanager cleanup

2015-02-24 Thread Stephan Erb
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31350/ --- Review request for Aurora. Repository: aurora Description --- Fix cluste