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 https://reviews.apache.org/r/31376/diff/1/?file=874379#file874379line149 Have you filed an upstream bug about this behaviour? I

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 https://reviews.apache.org/r/31376/diff/1/?file=874378#file874378line57 I don't think we should pass around any mutable state. I think data objects like

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.

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 https://reviews.apache.org/r/30891/diff/7/?file=873086#file873086line255 s/may/will/? Changed. - Maxim

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:

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 https://reviews.apache.org/r/30891/diff/7/?file=873086#file873086line301 No need to hold the intrinsic lock while logging here Having an explicit lock will

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

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!

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 https://reviews.apache.org/r/31376/diff/1/?file=874378#file874378line57 Would it make sense to accept/return `JobUpdateSummary`? The mutable/immutable

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 support

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 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!

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.,

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.

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 https://reviews.apache.org/r/31235/diff/2/?file=873240#file873240line63 This TODO wasn't addressed - put it back? Maxim Khutornenko wrote:

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

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

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.

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.

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 support

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 ---

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.,

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 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 support

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 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 support

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!

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,

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. There may

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 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.

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 https://reviews.apache.org/r/30710/diff/3/?file=861559#file861559line114 This is breaking Resources/Offer encapsulation and is not addressing

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.,

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.

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 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

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:

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.,

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:

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 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 support

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.,

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 conditions

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 https://reviews.apache.org/r/30710/diff/3/?file=861559#file861559line114 This is breaking Resources/Offer encapsulation and is not addressing

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 https://reviews.apache.org/r/30710/diff/3/?file=861559#file861559line174 Prefer using guava `Ordering` [1] instead. It will avoid excessive

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

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