Review Request 27996: Upgrade to gradle 2.2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/ --- Review request for Aurora, Joshua Cohen and Kevin Sweeney. Repository: aurora Description --- Release notes: http://www.gradle.org/docs/current/release-notes Biggest feature i spotted for us is ability to configure the VCS used in the idea plugin (exercised in this diff), saving us a manual step. Diffs - build.gradle 9ea7868cd47d4e83f74ca25f3dd861b9085fb26d gradle/wrapper/gradle-wrapper.jar 3d0dee6e8edfecc92e04653ec780de06f7b34f8b gradle/wrapper/gradle-wrapper.properties 9c0c9d387f22870bb5b4def0768a0c2db2f22ca8 Diff: https://reviews.apache.org/r/27996/diff/ Testing --- ./gradlew build -Pq git clean -fdx; ./gradlew idea, didn't need to set project VCS on the idea project. Thanks, Bill Farner
Re: Review Request 27996: Upgrade to gradle 2.2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/#review61269 --- This patch does not apply cleanly on master (6950d50), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 13, 2014, 5:05 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/ --- (Updated Nov. 13, 2014, 5:05 p.m.) Review request for Aurora, Joshua Cohen and Kevin Sweeney. Repository: aurora Description --- Release notes: http://www.gradle.org/docs/current/release-notes Biggest feature i spotted for us is ability to configure the VCS used in the idea plugin (exercised in this diff), saving us a manual step. Diffs - build.gradle 9ea7868cd47d4e83f74ca25f3dd861b9085fb26d gradle/wrapper/gradle-wrapper.jar 3d0dee6e8edfecc92e04653ec780de06f7b34f8b gradle/wrapper/gradle-wrapper.properties 9c0c9d387f22870bb5b4def0768a0c2db2f22ca8 Diff: https://reviews.apache.org/r/27996/diff/ Testing --- ./gradlew build -Pq git clean -fdx; ./gradlew idea, didn't need to set project VCS on the idea project. Thanks, Bill Farner
Re: Review Request 27947: Setting the max thread limit on AsyncEventBus.
On Nov. 13, 2014, 4:27 a.m., Kevin Sweeney wrote: src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java, line 98 https://reviews.apache.org/r/27947/diff/1/?file=760831#file760831line98 I don't think we want enqueuing a pubsubevent for processing to block, ever (otherwise we open ourselves to the possibility of deadlocks here). Consider using a LinkedBlockingQueue instead here. As this queue is still bounded at MAX_INT we might further consider adding a https://docs.oracle.com/javase/7/docs/api/java/util/concurrent/RejectedExecutionHandler.html that logs an appropriate message and shuts down the scheduler to make debugging easier. This is actually how the Executors.newCachedThreadPool() configures the async bus executor now. The only difference in this diff is capping the maxPoolSize to something more sane than Integer.MAX_VALUE. Looking at the ThreadPoolExecutor and the SynchronousQueue implementation it's not going to block when threads are busy/exhausted. The SynchronousQueue.offer() is going to timeout immediately (return false) and the ThreadPoolExecutor.execute() will proceed straight to reject(), where the default AbortPolicy would result in a RejectedExecutionException thrown in a thread calling EventBus.post(). So, the debugging story should be addressed well here. What is not addressed though is the fact that we are going to fail on start any time producers outrun consumers - pretty much guaranteed in a large cluster. I am going to switch to the newFixedThreadPool() with a few threads as Bill suggested. It already uses LinkedBlockingQueue and it's unlikely that we ever exceed Integer.MAX_VALUE on queueing side with multiple consuming threads. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/#review61203 --- On Nov. 13, 2014, 1:12 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/ --- (Updated Nov. 13, 2014, 1:12 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-926 https://issues.apache.org/jira/browse/AURORA-926 Repository: aurora Description --- Setting the max thread limit on AsyncEventBus. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java 24cd750c676a280d1cc953c96df126f19fc478f2 Diff: https://reviews.apache.org/r/27947/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 27947: Setting the max thread limit on AsyncEventBus.
On Nov. 13, 2014, 2:07 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java, line 93 https://reviews.apache.org/r/27947/diff/1/?file=760831#file760831line93 Given that we're moving from an effective same thread executor to async, i think we can get by with fewer knobs for now. How about: Executors.newFixedThreadPool( PUBSUB_WORKER_THREADS.get(), ... Maxim Khutornenko wrote: That was my first choice but ulimately decided in favor of the current approach to avoid a fixed memory tax of 1Mb X num_threads. With the current setting of 500 that would result in a constant waste of 500MB of JVM heap even when there is no event pressure. Bill Farner wrote: Good point. I'd actually suggest reducing the thread count dramatically, though, maybe 4 by default. We've been doing well with 1 for a long time, and throughput isn't driving the need for the async bus. I also wonder if we should not use `SynchronousQueue`. Since we're bounding the number of threads, busy threads means we'll force the work producer to wait for an available consumer. I think in that case we'd rather queue the work item and let the submitter proceed. What's your take? Agree, see my reply to Kevin for reasoning behind this. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/#review61178 --- On Nov. 13, 2014, 1:12 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/ --- (Updated Nov. 13, 2014, 1:12 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-926 https://issues.apache.org/jira/browse/AURORA-926 Repository: aurora Description --- Setting the max thread limit on AsyncEventBus. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java 24cd750c676a280d1cc953c96df126f19fc478f2 Diff: https://reviews.apache.org/r/27947/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 27996: Upgrade to gradle 2.2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/#review61272 --- Ship it! Ship It! - Joshua Cohen On Nov. 13, 2014, 5:05 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/ --- (Updated Nov. 13, 2014, 5:05 p.m.) Review request for Aurora, Joshua Cohen and Kevin Sweeney. Repository: aurora Description --- Release notes: http://www.gradle.org/docs/current/release-notes Biggest feature i spotted for us is ability to configure the VCS used in the idea plugin (exercised in this diff), saving us a manual step. Diffs - build.gradle 9ea7868cd47d4e83f74ca25f3dd861b9085fb26d gradle/wrapper/gradle-wrapper.jar 3d0dee6e8edfecc92e04653ec780de06f7b34f8b gradle/wrapper/gradle-wrapper.properties 9c0c9d387f22870bb5b4def0768a0c2db2f22ca8 Diff: https://reviews.apache.org/r/27996/diff/ Testing --- ./gradlew build -Pq git clean -fdx; ./gradlew idea, didn't need to set project VCS on the idea project. Thanks, Bill Farner
Re: Review Request 27996: Upgrade to gradle 2.2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/#review61273 --- Ship it! Ship It! - Maxim Khutornenko On Nov. 13, 2014, 5:05 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/ --- (Updated Nov. 13, 2014, 5:05 p.m.) Review request for Aurora, Joshua Cohen and Kevin Sweeney. Repository: aurora Description --- Release notes: http://www.gradle.org/docs/current/release-notes Biggest feature i spotted for us is ability to configure the VCS used in the idea plugin (exercised in this diff), saving us a manual step. Diffs - build.gradle 9ea7868cd47d4e83f74ca25f3dd861b9085fb26d gradle/wrapper/gradle-wrapper.jar 3d0dee6e8edfecc92e04653ec780de06f7b34f8b gradle/wrapper/gradle-wrapper.properties 9c0c9d387f22870bb5b4def0768a0c2db2f22ca8 Diff: https://reviews.apache.org/r/27996/diff/ Testing --- ./gradlew build -Pq git clean -fdx; ./gradlew idea, didn't need to set project VCS on the idea project. Thanks, Bill Farner
Re: Review Request 27947: Setting the max thread limit on AsyncEventBus.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/ --- (Updated Nov. 13, 2014, 5:41 p.m.) Review request for Aurora and Bill Farner. Changes --- CR comments. Bugs: AURORA-926 https://issues.apache.org/jira/browse/AURORA-926 Repository: aurora Description --- Setting the max thread limit on AsyncEventBus. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java 24cd750c676a280d1cc953c96df126f19fc478f2 Diff: https://reviews.apache.org/r/27947/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 27954: Fix bad review text in review bot.
On Nov. 13, 2014, 4:29 a.m., Kevin Sweeney wrote: build-support/jenkins/review_feedback.py, line 178 https://reviews.apache.org/r/27954/diff/1/?file=760871#file760871line178 prefer interpolation with % review_text over string concatenation +1 - Joshua --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27954/#review61204 --- On Nov. 13, 2014, 2:03 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27954/ --- (Updated Nov. 13, 2014, 2:03 a.m.) Review request for Aurora and Maxim Khutornenko. Repository: aurora Description --- Addressing bad output displayed in https://reviews.apache.org/r/27947/ Diffs - build-support/jenkins/review_feedback.py eaccfa81c323c9f5d284f16d5836e848a05bcb6d Diff: https://reviews.apache.org/r/27954/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 27947: Setting the max thread limit on AsyncEventBus.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/#review61278 --- %s 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 Nov. 13, 2014, 5:41 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/ --- (Updated Nov. 13, 2014, 5:41 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-926 https://issues.apache.org/jira/browse/AURORA-926 Repository: aurora Description --- Setting the max thread limit on AsyncEventBus. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java 24cd750c676a280d1cc953c96df126f19fc478f2 Diff: https://reviews.apache.org/r/27947/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 27947: Setting the max thread limit on AsyncEventBus.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/#review61279 --- Ship it! src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java https://reviews.apache.org/r/27947/#comment102819 Can you inject your own `LinkedBlockingQueue` and export its size in a metric? - Bill Farner On Nov. 13, 2014, 5:41 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/ --- (Updated Nov. 13, 2014, 5:41 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-926 https://issues.apache.org/jira/browse/AURORA-926 Repository: aurora Description --- Setting the max thread limit on AsyncEventBus. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java 24cd750c676a280d1cc953c96df126f19fc478f2 Diff: https://reviews.apache.org/r/27947/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 27954: Fix bad review text in review bot.
On Nov. 13, 2014, 4:29 a.m., Kevin Sweeney wrote: build-support/jenkins/review_feedback.py, line 178 https://reviews.apache.org/r/27954/diff/1/?file=760871#file760871line178 prefer interpolation with % review_text over string concatenation Joshua Cohen wrote: +1 Done. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27954/#review61204 --- On Nov. 13, 2014, 2:03 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27954/ --- (Updated Nov. 13, 2014, 2:03 a.m.) Review request for Aurora and Maxim Khutornenko. Repository: aurora Description --- Addressing bad output displayed in https://reviews.apache.org/r/27947/ Diffs - build-support/jenkins/review_feedback.py eaccfa81c323c9f5d284f16d5836e848a05bcb6d Diff: https://reviews.apache.org/r/27954/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 27954: Fix bad review text in review bot.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27954/ --- (Updated Nov. 13, 2014, 6:05 p.m.) Review request for Aurora and Kevin Sweeney. Repository: aurora Description --- Addressing bad output displayed in https://reviews.apache.org/r/27947/ Diffs (updated) - build-support/jenkins/review_feedback.py eaccfa81c323c9f5d284f16d5836e848a05bcb6d Diff: https://reviews.apache.org/r/27954/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 27996: Upgrade to gradle 2.2
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/#review61282 --- Ship it! Ship It! - Kevin Sweeney On Nov. 13, 2014, 9:05 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27996/ --- (Updated Nov. 13, 2014, 9:05 a.m.) Review request for Aurora, Joshua Cohen and Kevin Sweeney. Repository: aurora Description --- Release notes: http://www.gradle.org/docs/current/release-notes Biggest feature i spotted for us is ability to configure the VCS used in the idea plugin (exercised in this diff), saving us a manual step. Diffs - build.gradle 9ea7868cd47d4e83f74ca25f3dd861b9085fb26d gradle/wrapper/gradle-wrapper.jar 3d0dee6e8edfecc92e04653ec780de06f7b34f8b gradle/wrapper/gradle-wrapper.properties 9c0c9d387f22870bb5b4def0768a0c2db2f22ca8 Diff: https://reviews.apache.org/r/27996/diff/ Testing --- ./gradlew build -Pq git clean -fdx; ./gradlew idea, didn't need to set project VCS on the idea project. Thanks, Bill Farner
Re: Review Request 27848: Add friendly error message to the client when lock is held.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27848/ --- (Updated Nov. 13, 2014, 7:32 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Changes --- Rebase, isort. Bugs: AURORA-885 https://issues.apache.org/jira/browse/AURORA-885 Repository: aurora Description --- Add friendly error message to the client when lock is held. Diffs (updated) - src/main/python/apache/aurora/client/api/updater_util.py 2dd44e3a30202c6359d52a3499aa09f507cc26ca src/main/python/apache/aurora/client/cli/__init__.py 83416e66a3b997e9e910d3f1c6873a0616a3f932 src/main/python/apache/aurora/client/cli/context.py a5ebbdc2ca37c5fd1813854d0a34f15511e6ca06 src/main/python/apache/aurora/client/cli/jobs.py 28f9475c5accb8c73cbc5f7a1010920479a0388e src/main/python/apache/aurora/client/cli/options.py 1f5cbb616828932742e6482867e2eca9401aad61 src/test/python/apache/aurora/client/cli/test_create.py 968c997d7ad2d26b0916ad0c2a4ac83974d5c81f src/test/python/apache/aurora/client/cli/test_kill.py 78f5f04507d7fe080a1ed5ddda692e52f66cc18d src/test/python/apache/aurora/client/cli/test_restart.py a8180a3264ac1aa2ade654985755a4dbe262dc47 src/test/python/apache/aurora/client/cli/test_supdate.py 09f6a85aebdbf0ad9c9816684f4574132205ee65 src/test/python/apache/aurora/client/cli/test_update.py a5e59e4924618ab97f18ea056ef8225e864a317d src/test/python/apache/aurora/client/cli/util.py 154fb3a7170ae81548fcbc9f3cdd6dcf9bf1942d Diff: https://reviews.apache.org/r/27848/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/isort-check ./build-support/python/checkstyle-check Thanks, David McLaughlin
Re: Review Request 27848: Add friendly error message to the client when lock is held.
On Nov. 13, 2014, 4:14 a.m., Kevin Sweeney wrote: src/test/python/apache/aurora/client/cli/test_restart.py, line 16 https://reviews.apache.org/r/27848/diff/3-5/?file=757413#file757413line16 strange place for an import - should be in the 3rdparty section. did you run isort-run? isort-check said it was fine. isort-run moved the import though. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27848/#review61201 --- On Nov. 13, 2014, 7:32 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27848/ --- (Updated Nov. 13, 2014, 7:32 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-885 https://issues.apache.org/jira/browse/AURORA-885 Repository: aurora Description --- Add friendly error message to the client when lock is held. Diffs - src/main/python/apache/aurora/client/api/updater_util.py 2dd44e3a30202c6359d52a3499aa09f507cc26ca src/main/python/apache/aurora/client/cli/__init__.py 83416e66a3b997e9e910d3f1c6873a0616a3f932 src/main/python/apache/aurora/client/cli/context.py a5ebbdc2ca37c5fd1813854d0a34f15511e6ca06 src/main/python/apache/aurora/client/cli/jobs.py 28f9475c5accb8c73cbc5f7a1010920479a0388e src/main/python/apache/aurora/client/cli/options.py 1f5cbb616828932742e6482867e2eca9401aad61 src/test/python/apache/aurora/client/cli/test_create.py 968c997d7ad2d26b0916ad0c2a4ac83974d5c81f src/test/python/apache/aurora/client/cli/test_kill.py 78f5f04507d7fe080a1ed5ddda692e52f66cc18d src/test/python/apache/aurora/client/cli/test_restart.py a8180a3264ac1aa2ade654985755a4dbe262dc47 src/test/python/apache/aurora/client/cli/test_supdate.py 09f6a85aebdbf0ad9c9816684f4574132205ee65 src/test/python/apache/aurora/client/cli/test_update.py a5e59e4924618ab97f18ea056ef8225e864a317d src/test/python/apache/aurora/client/cli/util.py 154fb3a7170ae81548fcbc9f3cdd6dcf9bf1942d Diff: https://reviews.apache.org/r/27848/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/isort-check ./build-support/python/checkstyle-check Thanks, David McLaughlin
Re: Review Request 27947: Setting the max thread limit on AsyncEventBus.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/ --- (Updated Nov. 13, 2014, 7:33 p.m.) Review request for Aurora and Bill Farner. Changes --- Refactored to expose a queue size stat. Moved DeadEvent handler into a private class to avoid Uncallable method org.apache.aurora.scheduler.events.PubsubEventModule$2.logDeadEvent(DeadEvent) defined in anonymous class findBugs error. Bugs: AURORA-926 https://issues.apache.org/jira/browse/AURORA-926 Repository: aurora Description --- Setting the max thread limit on AsyncEventBus. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java 24cd750c676a280d1cc953c96df126f19fc478f2 src/test/java/org/apache/aurora/scheduler/events/PubsubEventModuleTest.java af3f0e3dc4f845c464dd6d9fc9f6b28ca8ad8c36 Diff: https://reviews.apache.org/r/27947/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 27492: AURORA-617: Switch pants 3rdparty to use python_requirements
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27492/#review61303 --- @ReviewBot retry - Joshua Cohen On Nov. 4, 2014, 3:35 p.m., Dan Norris wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27492/ --- (Updated Nov. 4, 2014, 3:35 p.m.) Review request for Aurora and Kevin Sweeney. Bugs: AURORA-617 https://issues.apache.org/jira/browse/AURORA-617 Repository: aurora Description --- Convert python_requirements links in 3rdparty/python/BUILD to be in requirements.txt Diffs - 3rdparty/python/BUILD 76d0bc58a72cd84c43f053082ee58e9c582e5437 3rdparty/python/requirements.txt PRE-CREATION Diff: https://reviews.apache.org/r/27492/diff/ Testing --- ./pants src/test/python:all ...which failed on one test in test_gc_executor.py, but the same test is failing for me in master. Thanks, Dan Norris
Re: Review Request 27848: Add friendly error message to the client when lock is held.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27848/#review61304 --- @ReviewBot retry - David McLaughlin On Nov. 13, 2014, 7:32 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27848/ --- (Updated Nov. 13, 2014, 7:32 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-885 https://issues.apache.org/jira/browse/AURORA-885 Repository: aurora Description --- Add friendly error message to the client when lock is held. Diffs - src/main/python/apache/aurora/client/api/updater_util.py 2dd44e3a30202c6359d52a3499aa09f507cc26ca src/main/python/apache/aurora/client/cli/__init__.py 83416e66a3b997e9e910d3f1c6873a0616a3f932 src/main/python/apache/aurora/client/cli/context.py a5ebbdc2ca37c5fd1813854d0a34f15511e6ca06 src/main/python/apache/aurora/client/cli/jobs.py 28f9475c5accb8c73cbc5f7a1010920479a0388e src/main/python/apache/aurora/client/cli/options.py 1f5cbb616828932742e6482867e2eca9401aad61 src/test/python/apache/aurora/client/cli/test_create.py 968c997d7ad2d26b0916ad0c2a4ac83974d5c81f src/test/python/apache/aurora/client/cli/test_kill.py 78f5f04507d7fe080a1ed5ddda692e52f66cc18d src/test/python/apache/aurora/client/cli/test_restart.py a8180a3264ac1aa2ade654985755a4dbe262dc47 src/test/python/apache/aurora/client/cli/test_supdate.py 09f6a85aebdbf0ad9c9816684f4574132205ee65 src/test/python/apache/aurora/client/cli/test_update.py a5e59e4924618ab97f18ea056ef8225e864a317d src/test/python/apache/aurora/client/cli/util.py 154fb3a7170ae81548fcbc9f3cdd6dcf9bf1942d Diff: https://reviews.apache.org/r/27848/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/isort-check ./build-support/python/checkstyle-check Thanks, David McLaughlin
Re: Review Request 27947: Setting the max thread limit on AsyncEventBus.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/#review61306 --- Ship it! Ship It! - Kevin Sweeney On Nov. 13, 2014, 11:33 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27947/ --- (Updated Nov. 13, 2014, 11:33 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-926 https://issues.apache.org/jira/browse/AURORA-926 Repository: aurora Description --- Setting the max thread limit on AsyncEventBus. Diffs - src/main/java/org/apache/aurora/scheduler/events/PubsubEventModule.java 24cd750c676a280d1cc953c96df126f19fc478f2 src/test/java/org/apache/aurora/scheduler/events/PubsubEventModuleTest.java af3f0e3dc4f845c464dd6d9fc9f6b28ca8ad8c36 Diff: https://reviews.apache.org/r/27947/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 27492: AURORA-617: Switch pants 3rdparty to use python_requirements
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27492/#review61308 --- This patch does not apply cleanly on master (de5a9b7), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 4, 2014, 3:35 p.m., Dan Norris wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27492/ --- (Updated Nov. 4, 2014, 3:35 p.m.) Review request for Aurora and Kevin Sweeney. Bugs: AURORA-617 https://issues.apache.org/jira/browse/AURORA-617 Repository: aurora Description --- Convert python_requirements links in 3rdparty/python/BUILD to be in requirements.txt Diffs - 3rdparty/python/BUILD 76d0bc58a72cd84c43f053082ee58e9c582e5437 3rdparty/python/requirements.txt PRE-CREATION Diff: https://reviews.apache.org/r/27492/diff/ Testing --- ./pants src/test/python:all ...which failed on one test in test_gc_executor.py, but the same test is failing for me in master. Thanks, Dan Norris
Review Request 28003: Remove 0.6.0 CHANGELOG entries in prep for 0.6.0-RC2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28003/ --- Review request for Aurora and Jake Farrell. Repository: aurora Description --- Remove 0.6.0 CHANGELOG entries in prep for 0.6.0-RC2. Diffs - CHANGELOG 21ddae6480814ec497e00aa7e1fecf797b857fee Diff: https://reviews.apache.org/r/28003/diff/ Testing --- Thanks, Bill Farner
Review Request 28005: Reset CHANGELOG and .auroraversion in prep for 0.6.0-RC2.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28005/ --- Review request for Aurora and Jake Farrell. Repository: aurora Description --- Reset CHANGELOG and .auroraversion in prep for 0.6.0-RC2. Diffs - .auroraversion e8ff9d45c6326c28dae836a1409ec0c9b00fd75a CHANGELOG 21ddae6480814ec497e00aa7e1fecf797b857fee Diff: https://reviews.apache.org/r/28005/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 27848: Add friendly error message to the client when lock is held.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27848/#review61320 --- Ship it! Master (f8040b9) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 13, 2014, 9:10 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27848/ --- (Updated Nov. 13, 2014, 9:10 p.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-885 https://issues.apache.org/jira/browse/AURORA-885 Repository: aurora Description --- Add friendly error message to the client when lock is held. Diffs - src/main/python/apache/aurora/client/api/updater_util.py 2dd44e3a30202c6359d52a3499aa09f507cc26ca src/main/python/apache/aurora/client/cli/__init__.py 83416e66a3b997e9e910d3f1c6873a0616a3f932 src/main/python/apache/aurora/client/cli/context.py a5ebbdc2ca37c5fd1813854d0a34f15511e6ca06 src/main/python/apache/aurora/client/cli/jobs.py 28f9475c5accb8c73cbc5f7a1010920479a0388e src/main/python/apache/aurora/client/cli/options.py 1f5cbb616828932742e6482867e2eca9401aad61 src/test/python/apache/aurora/client/cli/test_create.py 968c997d7ad2d26b0916ad0c2a4ac83974d5c81f src/test/python/apache/aurora/client/cli/test_kill.py 78f5f04507d7fe080a1ed5ddda692e52f66cc18d src/test/python/apache/aurora/client/cli/test_restart.py a8180a3264ac1aa2ade654985755a4dbe262dc47 src/test/python/apache/aurora/client/cli/test_supdate.py 09f6a85aebdbf0ad9c9816684f4574132205ee65 src/test/python/apache/aurora/client/cli/test_update.py a5e59e4924618ab97f18ea056ef8225e864a317d src/test/python/apache/aurora/client/cli/util.py 154fb3a7170ae81548fcbc9f3cdd6dcf9bf1942d Diff: https://reviews.apache.org/r/27848/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/isort-check ./build-support/python/checkstyle-check Thanks, David McLaughlin
Re: Review Request 26899: Require StateManager callers to open their own transactions.
On Oct. 21, 2014, 1:25 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, lines 817-819 https://reviews.apache.org/r/26899/diff/1-2/?file=724971#file724971line817 FWIW |= is a standard way to record boolean True in a result. This feels lees readable in my opinion. Sure thing, reverted. On Oct. 21, 2014, 1:25 a.m., Maxim Khutornenko wrote: src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java, line 825 https://reviews.apache.org/r/26899/diff/1-2/?file=724971#file724971line825 Would be great to update unit tests to validate this change. Which change are you thinking of? There's `testKillCronJob` to cover the cron case, if that's what you're referring to. Jacoco shows no branches missed here. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26899/#review57512 --- On Oct. 21, 2014, 1:09 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26899/ --- (Updated Oct. 21, 2014, 1:09 a.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack. This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read-write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes. [1] https://issues.apache.org/jira/browse/AURORA-871 Diffs - src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java 250c2df8113adfd62b3a7e124f7994156c82b5f7 src/main/java/org/apache/aurora/scheduler/async/Preemptor.java e9f251508257cd7287ff00773e0073a3cd130df8 src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java 1189ed0318ae0cf9663f0fa41775c4dd625bb397 src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234 src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 89140223f4f3acd02ade6fb95734744ef19d89bc src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 21cfebdf11e0652e192cf08e35c8581b1246f7b5 src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 58b94c2f2f3bac00f0692579974e8bdf159b6e40 src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 4db9be86f2e7db08d12e0182914a7c5130301b13 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java e792d23d6bb13b4e61b078beea6d063f72f0d8fc src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java 6ec130f4a9a5075b34452efb27c8fd0f08f93a63 src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 083a63543e5f9041f13fc6be66877f7173a5bf32 src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 8ee84cda8670d117e2efa2d1a114da6f0d8315d6 src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java b3e4ae39067b1dfb632f5d685d69fcbd7d4705da src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 6534329a92bf005223fa8907cbe4a8a3a511e142 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 9970948bace4c0ecbc51d6fc79270d77fb17bf87 src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java e79327c3b8ead01495e063e5c0e9270568e16f69 src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4d96761e04a342ad3564bdba4afdc889f27ac123
Re: Review Request 26899: Require StateManager callers to open their own transactions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26899/ --- (Updated Nov. 13, 2014, 10:19 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Changes --- Other priorities caused this review to go stale, so the inter-diff is probably not useful any more :-/ Repository: aurora Description --- Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack. This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read-write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes. [1] https://issues.apache.org/jira/browse/AURORA-871 Diffs (updated) - src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java e1b7d05b32bc5f6061e89fe2af901e34650f97bc src/main/java/org/apache/aurora/scheduler/async/Preemptor.java 1d337f6245530f0e22f75d5f1d301462d53dd8bb src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java e2ba8b8fe978a58d1edcd01963ea020e54529353 src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234 src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 8217c512f31d4222e855218bee8d3172767d4ba4 src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 86440ebd30bfe96262f30769d74e50c21d86f1c2 src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6663555ef58541afb9b6634196f1c40aff94274c src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 77db411f338416787011868fad58986b3d614a2d src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b2b66acee9c0789f3660469d6d504b4510af5e79 src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java f918d150fd14b71db5c1d0f501423a8f05e87f71 src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 4673e8097dade38449278424d055ef92d0d1b275 src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 59bfbcb9912a10c0e7fb28b6993cae7de8004ffe src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java 9682c89ee1b8f10a93d5e536359df4f034cc9eee src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 0e8a98c60293b8b5c23039b3c480670ed54faeb1 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 41709023fa95e29e45143f52ead68900c5ca6642 src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 17295acc1e818ec59253dac93915efbe5c6f1d06 src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4739909d1315e39b8225e4a718820f2f5965e3cc src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java c602cbdfc09bbc7179cd4a10602f11a4c89ef605 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 8d69ae97e77e271e01935722a9a4b2b2d1099273 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f36e34cc56e56c8268cafa0a2af0b15feb3187e8 Diff: https://reviews.apache.org/r/26899/diff/ Testing --- ./gradlew build -Pq Thanks, Bill Farner
Re: Review Request 26899: Require StateManager callers to open their own transactions.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26899/#review61324 --- Ship it! Master (f8040b9) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 13, 2014, 10:19 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26899/ --- (Updated Nov. 13, 2014, 10:19 p.m.) Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. Repository: aurora Description --- Apologies for the large diff - `StateManager` has a large fan-out. This is a pure functional no-op - just moving the call to `storage.write` up in the stack. This change is to prepare for AURORA-871 [1], as well as generally make use of `StateManager` safer. With the API prior to this diff, it would be easy to unintentionally attempt a fateful read-write lock upgrade. This firms up the API by requiring the caller to bring their own transaction, removing the implementation detail that a write transaction is opened behind the scenes. [1] https://issues.apache.org/jira/browse/AURORA-871 Diffs - src/main/java/org/apache/aurora/scheduler/UserTaskLauncher.java e1b7d05b32bc5f6061e89fe2af901e34650f97bc src/main/java/org/apache/aurora/scheduler/async/Preemptor.java 1d337f6245530f0e22f75d5f1d301462d53dd8bb src/main/java/org/apache/aurora/scheduler/async/TaskHistoryPruner.java 345cd8959045302fe3711e22396f5f7244a88c44 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java e2ba8b8fe978a58d1edcd01963ea020e54529353 src/main/java/org/apache/aurora/scheduler/async/TaskThrottler.java ca6129c7e0225530336c88f91a1451892f2ce234 src/main/java/org/apache/aurora/scheduler/async/TaskTimeout.java 8217c512f31d4222e855218bee8d3172767d4ba4 src/main/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJob.java 9388657ccb904364e460ec612c3da562b8952d7e src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java 86440ebd30bfe96262f30769d74e50c21d86f1c2 src/main/java/org/apache/aurora/scheduler/state/StateManager.java 3a2fd279c2953d564b7fddabf31afda001bb3dfe src/main/java/org/apache/aurora/scheduler/state/StateManagerImpl.java 6663555ef58541afb9b6634196f1c40aff94274c src/main/java/org/apache/aurora/scheduler/state/TaskAssigner.java 77db411f338416787011868fad58986b3d614a2d src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b2b66acee9c0789f3660469d6d504b4510af5e79 src/main/java/org/apache/aurora/scheduler/updater/InstanceAction.java 3774c851b58e747acf25735d24334408b1c5386d src/main/java/org/apache/aurora/scheduler/updater/InstanceActionHandler.java f4363aa8ce9ef9f583b52251f351c8c971ef8119 src/main/java/org/apache/aurora/scheduler/updater/JobUpdateControllerImpl.java f918d150fd14b71db5c1d0f501423a8f05e87f71 src/test/java/org/apache/aurora/scheduler/UserTaskLauncherTest.java 4673e8097dade38449278424d055ef92d0d1b275 src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java 59bfbcb9912a10c0e7fb28b6993cae7de8004ffe src/test/java/org/apache/aurora/scheduler/async/TaskHistoryPrunerTest.java 9682c89ee1b8f10a93d5e536359df4f034cc9eee src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerImplTest.java 0e8a98c60293b8b5c23039b3c480670ed54faeb1 src/test/java/org/apache/aurora/scheduler/async/TaskSchedulerTest.java 41709023fa95e29e45143f52ead68900c5ca6642 src/test/java/org/apache/aurora/scheduler/async/TaskThrottlerTest.java a28e512afde584fb94ff7686d7a3e3fbb51f8b7b src/test/java/org/apache/aurora/scheduler/async/TaskTimeoutTest.java 17295acc1e818ec59253dac93915efbe5c6f1d06 src/test/java/org/apache/aurora/scheduler/cron/quartz/AuroraCronJobTest.java b6b1bcbf8080eb7d1e7eca4a486cc063f28db75d src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java 4739909d1315e39b8225e4a718820f2f5965e3cc src/test/java/org/apache/aurora/scheduler/state/StateManagerImplTest.java c602cbdfc09bbc7179cd4a10602f11a4c89ef605 src/test/java/org/apache/aurora/scheduler/state/TaskAssignerImplTest.java 8d69ae97e77e271e01935722a9a4b2b2d1099273 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c src/test/java/org/apache/aurora/scheduler/updater/AddTaskTest.java f9ed46f56bb11e9c158268c16f29557f3e99c84e src/test/java/org/apache/aurora/scheduler/updater/JobUpdaterIT.java f36e34cc56e56c8268cafa0a2af0b15feb3187e8 Diff: https://reviews.apache.org/r/26899/diff/
Re: Review Request 27852: Ensure run verb returns an exit code.
On Nov. 11, 2014, 2:55 p.m., Bill Farner wrote: src/main/python/apache/aurora/client/cli/task.py, line 72 https://reviews.apache.org/r/27852/diff/1/?file=757426#file757426line72 Should we just make this the default behavior? There's at least 31 locations that do this, seems like a catch-all would be useful. If we do this, i'd recommend you remove all `return EXIT_OK` lines, so only proceed if you're okay with that. Looks like the relevant changes would be in: src/main/python/apache/aurora/client/cli/standalone_client.py src/main/python/apache/aurora/client/cli/client.py Zameer Manji wrote: I don't see how this approach will work. This problem of not returning an exit code comes from the subclasses not implementing the method correctly not that we dispatch to the super class's implementation. In addition I'm not comfortable mixing in this (minor) change with a larger structural change. Bill Farner wrote: Are we talking about different things? I'm just suggesting a change like this, around sys.exit: ``` client = AuroraCommandLine() if len(sys.argv) == 1: sys.argv.append(help) - sys.exit(client.execute(sys.argv[1:])) + exit_code = client.execute(sys.argv[1:]) + sys.exit(0 if exit_code is None else exit_code) if __name__ == '__main__': proxy_main() ``` This is basically just changing the contract to exit 0 if no explicit exit code is returned. Given we're working in a language that doesn't have a compiler to enforce a return value, i think this is the right thing to do. The python philisohpy is 'explicit over implicit'. I feel that returning None is an error (that we should catch in our tests) and we should explicitly returning the exit code. - Zameer --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/#review60888 --- On Nov. 10, 2014, 6:50 p.m., Zameer Manji wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/ --- (Updated Nov. 10, 2014, 6:50 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-923 https://issues.apache.org/jira/browse/AURORA-923 Repository: aurora Description --- Ensure run verb returns an exit code. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b Diff: https://reviews.apache.org/r/27852/diff/ Testing --- ./pants build src/test/python/apache/aurora/client/cli:: Thanks, Zameer Manji
Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/ --- (Updated Nov. 14, 2014, 12:30 a.m.) Review request for Aurora, Bill Farner and Zameer Manji. Changes --- CR comments. Bugs: AURORA-914 https://issues.apache.org/jira/browse/AURORA-914 Repository: aurora Description --- Adding @Timed to trace scheduling latencies and Veto counters per type. Diffs (updated) - src/main/java/org/apache/aurora/scheduler/TaskVars.java cf8f7584afee758c527798914181049051aef0d8 src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java d2682cd910d248c897e691bcb4c8a3a6f1aec2d2 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java e2ba8b8fe978a58d1edcd01963ea020e54529353 src/main/java/org/apache/aurora/scheduler/filter/ConstraintFilter.java 3839083f27ca5d4b93406152559b58b04e912a10 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java c1c5f26723f1eac3000e09e061b4582f922fded6 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java cc6b53b3265253f76c1e954c0108aa5936f5cc36 src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 87203690f09456ac1ca5e9da2b82826d60cbd723 src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java aaedb3b5ec2cb27550449435efa8f335c6a9baad src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 12ea4c67350c2992f59bacd21a99d1413b60b757 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java 94f0a179b786649775899f855f7c1a0caab7290f src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java e113eba1f304279b5ee3d70db1d1ea558efd63ac src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java b60b004adbd6753ec6fef125fd70286be5071c56 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c Diff: https://reviews.apache.org/r/27705/diff/ Testing --- ./gradlew -Pq build Verified new stats in vagrant. Thanks, Maxim Khutornenko
Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.
On Nov. 11, 2014, 10:04 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java, line 360 https://reviews.apache.org/r/27705/diff/1/?file=753241#file753241line360 It's nice when boolean-returning methods have names that self-document what the return value is. In this case, something like `accceptsOffer`. Renamed. On Nov. 11, 2014, 10:04 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java, line 362 https://reviews.apache.org/r/27705/diff/1/?file=753241#file753241line362 odd line break here, don't split up the type parameter. no idea how that happened, fixed. On Nov. 11, 2014, 10:04 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java, line 81 https://reviews.apache.org/r/27705/diff/1/?file=753244#file753244line81 s/rawReason/reasonParameter/? Additionally, this should probably be `String...` to avoid a future yak shave to support non-unary argument vetoes. Renamed. I don't see a use case for String... just yet. Given this is a private constructor it should be easy to add when needed. On Nov. 11, 2014, 10:04 p.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java, line 283 https://reviews.apache.org/r/27705/diff/1/?file=753245#file753245line283 Can you do this in a SchedulingFilter decorator instead? That way you can hide a bunch of logic from SchedulingFilterImpl. Bill Farner wrote: Oh, looks like the hard work may already be done - can you consume the `Vetoed` pubsub event elsewhere? Sure, moved to TaskVars. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/#review60861 --- On Nov. 6, 2014, 10:23 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/ --- (Updated Nov. 6, 2014, 10:23 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-914 https://issues.apache.org/jira/browse/AURORA-914 Repository: aurora Description --- Adding @Timed to trace scheduling latencies and Veto counters per type. Diffs - src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java 1a45d08c8854a335161476c8321c751f763dc12e src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java b23457e0e64b490297166131a1b1b51b6d330415 src/main/java/org/apache/aurora/scheduler/filter/ConstraintFilter.java 3839083f27ca5d4b93406152559b58b04e912a10 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java c37272c9f46c086cb57b79a5202b3bd80e156f07 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java 0533baa5e90ca62b8d35ba05474eaa8e27741a5a src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 87203690f09456ac1ca5e9da2b82826d60cbd723 src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java aaedb3b5ec2cb27550449435efa8f335c6a9baad src/test/java/org/apache/aurora/scheduler/async/PreemptorImplTest.java c0fa462c0ebe0b06fa354f5f63d5965827c669a1 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java 0318179cd70661890f5a53908d1985d54474d476 src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java bffbf83653535ecd9bf7b149e1e564c5fba56d17 src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java b60b004adbd6753ec6fef125fd70286be5071c56 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java b42f6e2d23eb44605cc84ce01a80f10344a7e43f Diff: https://reviews.apache.org/r/27705/diff/ Testing --- ./gradlew -Pq build Verified new stats in vagrant. Thanks, Maxim Khutornenko
Re: Review Request 28019: Show update page URL in the output for beta-update start.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/#review61346 --- Ship it! Master (254e175) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 14, 2014, 12:19 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/ --- (Updated Nov. 14, 2014, 12:19 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-766 https://issues.apache.org/jira/browse/AURORA-766 Repository: aurora Description --- Show update page URL in the output for beta-update start. Diffs - src/main/python/apache/aurora/client/cli/context.py df9dc24a060ef551009f48687fe9ecd928ee04a4 src/main/python/apache/aurora/client/cli/update.py 30763920bbc0b7e7a19e9a1a873d72680b7f821b src/test/python/apache/aurora/client/cli/test_supdate.py 97a9664a5230a7331bd7b784cbbc958d4c07cbb1 Diff: https://reviews.apache.org/r/28019/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/checkstyle-check src/ ./build-support/python/isort-check src/ Thanks, David McLaughlin
Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/#review61351 --- Ship it! Master (254e175) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 14, 2014, 12:30 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/ --- (Updated Nov. 14, 2014, 12:30 a.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-914 https://issues.apache.org/jira/browse/AURORA-914 Repository: aurora Description --- Adding @Timed to trace scheduling latencies and Veto counters per type. Diffs - src/main/java/org/apache/aurora/scheduler/TaskVars.java cf8f7584afee758c527798914181049051aef0d8 src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java d2682cd910d248c897e691bcb4c8a3a6f1aec2d2 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java e2ba8b8fe978a58d1edcd01963ea020e54529353 src/main/java/org/apache/aurora/scheduler/filter/ConstraintFilter.java 3839083f27ca5d4b93406152559b58b04e912a10 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java c1c5f26723f1eac3000e09e061b4582f922fded6 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java cc6b53b3265253f76c1e954c0108aa5936f5cc36 src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 87203690f09456ac1ca5e9da2b82826d60cbd723 src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java aaedb3b5ec2cb27550449435efa8f335c6a9baad src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 12ea4c67350c2992f59bacd21a99d1413b60b757 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java 94f0a179b786649775899f855f7c1a0caab7290f src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java e113eba1f304279b5ee3d70db1d1ea558efd63ac src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java b60b004adbd6753ec6fef125fd70286be5071c56 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c Diff: https://reviews.apache.org/r/27705/diff/ Testing --- ./gradlew -Pq build Verified new stats in vagrant. Thanks, Maxim Khutornenko
Re: Review Request 27698: Move zookeeper connection off the main thread to prevent client deadlocks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27698/ --- (Updated Nov. 14, 2014, 1:03 a.m.) Review request for Aurora, Kevin Sweeney, Brian Wickman, and Zameer Manji. Changes --- rb feedback. Repository: aurora Description --- The underlying Kazoo code here has an uninterruptable wait on the main thread, which means if there are any problems establishing a connection then you need to hard kill the client process. Moving the code to a separate thread. Diffs (updated) - src/main/python/apache/aurora/client/api/scheduler_client.py 3a851cc9f53380b4bf2e9df47080e7c476c3e38e src/test/python/apache/aurora/client/api/test_scheduler_client.py 1f1c6e0ab686cc63bf0a6b7a36e7eef688644e74 Diff: https://reviews.apache.org/r/27698/diff/ Testing --- ./pants src/test/python/apache/aurora/client:all src.test.python.apache.aurora.client.api.api . SUCCESS src.test.python.apache.aurora.client.api.disambiguator . SUCCESS src.test.python.apache.aurora.client.api.instance_watcher . SUCCESS src.test.python.apache.aurora.client.api.job_monitor . SUCCESS src.test.python.apache.aurora.client.api.mux . SUCCESS src.test.python.apache.aurora.client.api.quota_check . SUCCESS src.test.python.apache.aurora.client.api.restarter . SUCCESS src.test.python.apache.aurora.client.api.scheduler_client . SUCCESS src.test.python.apache.aurora.client.api.sla . SUCCESS src.test.python.apache.aurora.client.api.updater . SUCCESS src.test.python.apache.aurora.client.api.updater_util . SUCCESS src.test.python.apache.aurora.client.binding_helper . SUCCESS src.test.python.apache.aurora.client.cli.api . SUCCESS src.test.python.apache.aurora.client.cli.bridge . SUCCESS src.test.python.apache.aurora.client.cli.command_hooks . SUCCESS src.test.python.apache.aurora.client.cli.config . SUCCESS src.test.python.apache.aurora.client.cli.cron . SUCCESS src.test.python.apache.aurora.client.cli.help . SUCCESS src.test.python.apache.aurora.client.cli.inspect . SUCCESS src.test.python.apache.aurora.client.cli.job . SUCCESS src.test.python.apache.aurora.client.cli.logging . SUCCESS src.test.python.apache.aurora.client.cli.plugins . SUCCESS src.test.python.apache.aurora.client.cli.quota . SUCCESS src.test.python.apache.aurora.client.cli.sla . SUCCESS src.test.python.apache.aurora.client.cli.supdate . SUCCESS src.test.python.apache.aurora.client.cli.task . SUCCESS src.test.python.apache.aurora.client.cli.update . SUCCESS src.test.python.apache.aurora.client.commands.admin . SUCCESS src.test.python.apache.aurora.client.commands.core . SUCCESS src.test.python.apache.aurora.client.commands.hooks . SUCCESS src.test.python.apache.aurora.client.commands.maintenance . SUCCESS src.test.python.apache.aurora.client.commands.run . SUCCESS src.test.python.apache.aurora.client.commands.ssh . SUCCESS src.test.python.apache.aurora.client.config . SUCCESS src.test.python.apache.aurora.client.hooks.hooked_api . SUCCESS src.test.python.apache.aurora.client.hooks.non_hooked_api . SUCCESS Thanks, David McLaughlin
Re: Review Request 27698: Move zookeeper connection off the main thread to prevent client deadlocks.
On Nov. 13, 2014, 12:46 a.m., Kevin Sweeney wrote: src/main/python/apache/aurora/client/api/scheduler_client.py, line 74 https://reviews.apache.org/r/27698/diff/3/?file=754744#file754744line74 Suggestion (other reviewers feel free to chime in here): instead of calling the constructor `deadline_fn` call it `_deadline` to indicate that overriding the deadline implementation isn't part of the public API. Also, push this down to ZooKeeperSchedulerClient - the base class does not use deadline. Bill Farner wrote: Works for me (private kwarg naming). Done. On Nov. 13, 2014, 12:46 a.m., Kevin Sweeney wrote: src/test/python/apache/aurora/client/api/test_scheduler_client.py, line 361 https://reviews.apache.org/r/27698/diff/3/?file=754745#file754745line361 replace this with a constructor kwarg on line 357 above to ensure you've sucessfully replaced deadline with a non-forking one. Done. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27698/#review61155 --- On Nov. 7, 2014, 9:17 p.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27698/ --- (Updated Nov. 7, 2014, 9:17 p.m.) Review request for Aurora, Kevin Sweeney, Brian Wickman, and Zameer Manji. Repository: aurora Description --- The underlying Kazoo code here has an uninterruptable wait on the main thread, which means if there are any problems establishing a connection then you need to hard kill the client process. Moving the code to a separate thread. Diffs - src/main/python/apache/aurora/client/api/scheduler_client.py 3a851cc9f53380b4bf2e9df47080e7c476c3e38e src/test/python/apache/aurora/client/api/test_scheduler_client.py 1f1c6e0ab686cc63bf0a6b7a36e7eef688644e74 Diff: https://reviews.apache.org/r/27698/diff/ Testing --- ./pants src/test/python/apache/aurora/client:all src.test.python.apache.aurora.client.api.api . SUCCESS src.test.python.apache.aurora.client.api.disambiguator . SUCCESS src.test.python.apache.aurora.client.api.instance_watcher . SUCCESS src.test.python.apache.aurora.client.api.job_monitor . SUCCESS src.test.python.apache.aurora.client.api.mux . SUCCESS src.test.python.apache.aurora.client.api.quota_check . SUCCESS src.test.python.apache.aurora.client.api.restarter . SUCCESS src.test.python.apache.aurora.client.api.scheduler_client . SUCCESS src.test.python.apache.aurora.client.api.sla . SUCCESS src.test.python.apache.aurora.client.api.updater . SUCCESS src.test.python.apache.aurora.client.api.updater_util . SUCCESS src.test.python.apache.aurora.client.binding_helper . SUCCESS src.test.python.apache.aurora.client.cli.api . SUCCESS src.test.python.apache.aurora.client.cli.bridge . SUCCESS src.test.python.apache.aurora.client.cli.command_hooks . SUCCESS src.test.python.apache.aurora.client.cli.config . SUCCESS src.test.python.apache.aurora.client.cli.cron . SUCCESS src.test.python.apache.aurora.client.cli.help . SUCCESS src.test.python.apache.aurora.client.cli.inspect . SUCCESS src.test.python.apache.aurora.client.cli.job . SUCCESS src.test.python.apache.aurora.client.cli.logging . SUCCESS src.test.python.apache.aurora.client.cli.plugins . SUCCESS src.test.python.apache.aurora.client.cli.quota . SUCCESS src.test.python.apache.aurora.client.cli.sla . SUCCESS src.test.python.apache.aurora.client.cli.supdate . SUCCESS src.test.python.apache.aurora.client.cli.task . SUCCESS src.test.python.apache.aurora.client.cli.update . SUCCESS src.test.python.apache.aurora.client.commands.admin
Review Request 28021: Remove dependency on commons-lang.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28021/ --- Review request for Aurora, Joshua Cohen and Kevin Sweeney. Repository: aurora Description --- Remove dependency on commons-lang. Diffs - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 3661f8487985f631e3ea437fe6430e0296376a9e src/main/java/org/apache/aurora/scheduler/configuration/SanitizedConfiguration.java 4eb4437b91906ae191dd105474e84ba5f40cf52e src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java 48b403e155684861ec0307a61ccb0775cdb57a32 src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 37176237fac336413267f3c8bb4e1b9a6255150c src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 Diff: https://reviews.apache.org/r/28021/diff/ Testing --- ./gradlew clean build -Pq Thanks, Zameer Manji
Re: Review Request 27601: Adding resource consumption calculation for cron jobs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27601/#review61362 --- @ReviewBot retry - Maxim Khutornenko On Nov. 5, 2014, 1:23 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27601/ --- (Updated Nov. 5, 2014, 1:23 a.m.) Review request for Aurora, Kevin Sweeney and Bill Farner. Bugs: AURORA-825 https://issues.apache.org/jira/browse/AURORA-825 Repository: aurora Description --- Fixing cron template filtering issue. This is the revision of the original implementation submitted in https://reviews.apache.org/r/27317 and reverted shorly after. Diffs - src/main/java/org/apache/aurora/scheduler/quota/QuotaInfo.java d4e0f5335c9ebd8da97cb9634152b46c2bd9affe src/main/java/org/apache/aurora/scheduler/quota/QuotaManager.java 3fbab8b27403a416e86c97970eb6124c8a34c38c src/test/java/org/apache/aurora/scheduler/quota/QuotaManagerImplTest.java 2fe257416e8fc11be3ebf0dabbf4f63628b3c670 Diff: https://reviews.apache.org/r/27601/diff/ Testing --- ./gradlew -Pq build Also, created multiple role cron jobs in vagrant to validate filtering. Thanks, Maxim Khutornenko
Re: Review Request 28019: Show update page URL in the output for beta-update start.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/#review61364 --- src/main/python/apache/aurora/client/cli/update.py https://reviews.apache.org/r/28019/#comment102941 How about combining the two into something like: Job update has started. View update progress at %s? - Maxim Khutornenko On Nov. 14, 2014, 12:19 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/ --- (Updated Nov. 14, 2014, 12:19 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-766 https://issues.apache.org/jira/browse/AURORA-766 Repository: aurora Description --- Show update page URL in the output for beta-update start. Diffs - src/main/python/apache/aurora/client/cli/context.py df9dc24a060ef551009f48687fe9ecd928ee04a4 src/main/python/apache/aurora/client/cli/update.py 30763920bbc0b7e7a19e9a1a873d72680b7f821b src/test/python/apache/aurora/client/cli/test_supdate.py 97a9664a5230a7331bd7b784cbbc958d4c07cbb1 Diff: https://reviews.apache.org/r/28019/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/checkstyle-check src/ ./build-support/python/isort-check src/ Thanks, David McLaughlin
Review Request 28026: Add more test coverage to SchedulerThriftInterface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28026/ --- Review request for Aurora, David McLaughlin and Zameer Manji. Repository: aurora Description --- This brings SchedulerThriftInterface to 99% instruction coverage and 96% branch coverage. The remaining pieces are legitimately difficult to capture. I also uncovered a few minor bugs along the way, most noteworthy being AOP interceptor ordering leading to the standard API response not always being applied. Additionally, i removed a bunch of unnecessary exception handling that is now done by an AOP interceptor. Diffs - src/main/java/org/apache/aurora/auth/SessionValidator.java eeebb78901a6c33e08ceb8e675c91f0b5f44bcbc src/main/java/org/apache/aurora/scheduler/storage/backup/Recovery.java 4744dc9f202969906113ccb610bf17c94d188c43 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b2b66acee9c0789f3660469d6d504b4510af5e79 src/main/java/org/apache/aurora/scheduler/thrift/Util.java 18e2bdfab6406761d7e7cbcec26d12fb28441fe0 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java dca855c522d21924821fc47e636da39689aec4b7 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c Diff: https://reviews.apache.org/r/28026/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28021: Remove dependency on commons-lang.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28021/#review61368 --- Master (254e175) 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 Nov. 14, 2014, 1:07 a.m., Zameer Manji wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28021/ --- (Updated Nov. 14, 2014, 1:07 a.m.) Review request for Aurora, Joshua Cohen and Kevin Sweeney. Repository: aurora Description --- Remove dependency on commons-lang. Diffs - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 3661f8487985f631e3ea437fe6430e0296376a9e src/main/java/org/apache/aurora/scheduler/configuration/SanitizedConfiguration.java 4eb4437b91906ae191dd105474e84ba5f40cf52e src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java 48b403e155684861ec0307a61ccb0775cdb57a32 src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 37176237fac336413267f3c8bb4e1b9a6255150c src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 Diff: https://reviews.apache.org/r/28021/diff/ Testing --- ./gradlew clean build -Pq Thanks, Zameer Manji
Re: Review Request 28019: Show update page URL in the output for beta-update start.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/ --- (Updated Nov. 14, 2014, 1:44 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Changes --- Simplify. Bugs: AURORA-766 https://issues.apache.org/jira/browse/AURORA-766 Repository: aurora Description --- Show update page URL in the output for beta-update start. Diffs (updated) - src/main/python/apache/aurora/client/cli/context.py df9dc24a060ef551009f48687fe9ecd928ee04a4 src/main/python/apache/aurora/client/cli/update.py 30763920bbc0b7e7a19e9a1a873d72680b7f821b src/test/python/apache/aurora/client/cli/test_supdate.py 97a9664a5230a7331bd7b784cbbc958d4c07cbb1 Diff: https://reviews.apache.org/r/28019/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/checkstyle-check src/ ./build-support/python/isort-check src/ Thanks, David McLaughlin
Re: Review Request 28019: Show update page URL in the output for beta-update start.
On Nov. 14, 2014, 1:22 a.m., Maxim Khutornenko wrote: src/main/python/apache/aurora/client/cli/update.py, lines 83-85 https://reviews.apache.org/r/28019/diff/1/?file=763026#file763026line83 How about combining the two into something like: Job update has started. View update progress at %s? Done. - David --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/#review61364 --- On Nov. 14, 2014, 12:19 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/ --- (Updated Nov. 14, 2014, 12:19 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-766 https://issues.apache.org/jira/browse/AURORA-766 Repository: aurora Description --- Show update page URL in the output for beta-update start. Diffs - src/main/python/apache/aurora/client/cli/context.py df9dc24a060ef551009f48687fe9ecd928ee04a4 src/main/python/apache/aurora/client/cli/update.py 30763920bbc0b7e7a19e9a1a873d72680b7f821b src/test/python/apache/aurora/client/cli/test_supdate.py 97a9664a5230a7331bd7b784cbbc958d4c07cbb1 Diff: https://reviews.apache.org/r/28019/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/checkstyle-check src/ ./build-support/python/isort-check src/ Thanks, David McLaughlin
Re: Review Request 28021: Remove dependency on commons-lang.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28021/#review61372 --- Ship it! Ship It! - Bill Farner On Nov. 14, 2014, 1:07 a.m., Zameer Manji wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28021/ --- (Updated Nov. 14, 2014, 1:07 a.m.) Review request for Aurora, Joshua Cohen and Kevin Sweeney. Repository: aurora Description --- Remove dependency on commons-lang. Diffs - build.gradle 3237f8dfa3e7d4249a388042dba840a939d513b3 src/main/java/org/apache/aurora/scheduler/configuration/ConfigurationManager.java 3661f8487985f631e3ea437fe6430e0296376a9e src/main/java/org/apache/aurora/scheduler/configuration/SanitizedConfiguration.java 4eb4437b91906ae191dd105474e84ba5f40cf52e src/main/java/org/apache/aurora/scheduler/cron/SanitizedCronJob.java 48b403e155684861ec0307a61ccb0775cdb57a32 src/main/java/org/apache/aurora/scheduler/storage/mem/MemTaskStore.java 37176237fac336413267f3c8bb4e1b9a6255150c src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java a43e5d7748c22d60f56f03a8a3d52949faebeff2 Diff: https://reviews.apache.org/r/28021/diff/ Testing --- ./gradlew clean build -Pq Thanks, Zameer Manji
Re: Review Request 28026: Add more test coverage to SchedulerThriftInterface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28026/#review61371 --- src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java https://reviews.apache.org/r/28026/#comment102945 Can you change this if you agree with https://reviews.apache.org/r/28021/ ? src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java https://reviews.apache.org/r/28026/#comment102948 Can you file a JIRA for this? - Zameer Manji On Nov. 13, 2014, 5:30 p.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28026/ --- (Updated Nov. 13, 2014, 5:30 p.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Repository: aurora Description --- This brings SchedulerThriftInterface to 99% instruction coverage and 96% branch coverage. The remaining pieces are legitimately difficult to capture. I also uncovered a few minor bugs along the way, most noteworthy being AOP interceptor ordering leading to the standard API response not always being applied. Additionally, i removed a bunch of unnecessary exception handling that is now done by an AOP interceptor. Diffs - src/main/java/org/apache/aurora/auth/SessionValidator.java eeebb78901a6c33e08ceb8e675c91f0b5f44bcbc src/main/java/org/apache/aurora/scheduler/storage/backup/Recovery.java 4744dc9f202969906113ccb610bf17c94d188c43 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b2b66acee9c0789f3660469d6d504b4510af5e79 src/main/java/org/apache/aurora/scheduler/thrift/Util.java 18e2bdfab6406761d7e7cbcec26d12fb28441fe0 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java dca855c522d21924821fc47e636da39689aec4b7 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c Diff: https://reviews.apache.org/r/28026/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28019: Show update page URL in the output for beta-update start.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/#review61375 --- Ship it! - Maxim Khutornenko On Nov. 14, 2014, 1:44 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/ --- (Updated Nov. 14, 2014, 1:44 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-766 https://issues.apache.org/jira/browse/AURORA-766 Repository: aurora Description --- Show update page URL in the output for beta-update start. Diffs - src/main/python/apache/aurora/client/cli/context.py df9dc24a060ef551009f48687fe9ecd928ee04a4 src/main/python/apache/aurora/client/cli/update.py 30763920bbc0b7e7a19e9a1a873d72680b7f821b src/test/python/apache/aurora/client/cli/test_supdate.py 97a9664a5230a7331bd7b784cbbc958d4c07cbb1 Diff: https://reviews.apache.org/r/28019/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/checkstyle-check src/ ./build-support/python/isort-check src/ Thanks, David McLaughlin
Re: Review Request 28019: Show update page URL in the output for beta-update start.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/#review61383 --- Ship it! Ship It! - Bill Farner On Nov. 14, 2014, 1:44 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/ --- (Updated Nov. 14, 2014, 1:44 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-766 https://issues.apache.org/jira/browse/AURORA-766 Repository: aurora Description --- Show update page URL in the output for beta-update start. Diffs - src/main/python/apache/aurora/client/cli/context.py df9dc24a060ef551009f48687fe9ecd928ee04a4 src/main/python/apache/aurora/client/cli/update.py 30763920bbc0b7e7a19e9a1a873d72680b7f821b src/test/python/apache/aurora/client/cli/test_supdate.py 97a9664a5230a7331bd7b784cbbc958d4c07cbb1 Diff: https://reviews.apache.org/r/28019/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/checkstyle-check src/ ./build-support/python/isort-check src/ Thanks, David McLaughlin
Re: Review Request 28026: Add more test coverage to SchedulerThriftInterface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28026/#review61384 --- Ship it! Ship It! - Maxim Khutornenko On Nov. 14, 2014, 1:30 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28026/ --- (Updated Nov. 14, 2014, 1:30 a.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Repository: aurora Description --- This brings SchedulerThriftInterface to 99% instruction coverage and 96% branch coverage. The remaining pieces are legitimately difficult to capture. I also uncovered a few minor bugs along the way, most noteworthy being AOP interceptor ordering leading to the standard API response not always being applied. Additionally, i removed a bunch of unnecessary exception handling that is now done by an AOP interceptor. Diffs - src/main/java/org/apache/aurora/auth/SessionValidator.java eeebb78901a6c33e08ceb8e675c91f0b5f44bcbc src/main/java/org/apache/aurora/scheduler/storage/backup/Recovery.java 4744dc9f202969906113ccb610bf17c94d188c43 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b2b66acee9c0789f3660469d6d504b4510af5e79 src/main/java/org/apache/aurora/scheduler/thrift/Util.java 18e2bdfab6406761d7e7cbcec26d12fb28441fe0 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java dca855c522d21924821fc47e636da39689aec4b7 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c Diff: https://reviews.apache.org/r/28026/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/#review61385 --- src/main/java/org/apache/aurora/scheduler/TaskVars.java https://reviews.apache.org/r/27705/#comment102954 To get the data we want, some extra analysis is needed. Specifically - if we want to figure out how often a scheduling attempt is vetoed _only_ for static reasons (e.g. insufficient resources), these stats will lack signal. Instead, we probably want two counters: - scheduling_veto_static - scheduling_veto_dynamic Does that make sense? - Bill Farner On Nov. 14, 2014, 12:30 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/ --- (Updated Nov. 14, 2014, 12:30 a.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-914 https://issues.apache.org/jira/browse/AURORA-914 Repository: aurora Description --- Adding @Timed to trace scheduling latencies and Veto counters per type. Diffs - src/main/java/org/apache/aurora/scheduler/TaskVars.java cf8f7584afee758c527798914181049051aef0d8 src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java d2682cd910d248c897e691bcb4c8a3a6f1aec2d2 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java e2ba8b8fe978a58d1edcd01963ea020e54529353 src/main/java/org/apache/aurora/scheduler/filter/ConstraintFilter.java 3839083f27ca5d4b93406152559b58b04e912a10 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java c1c5f26723f1eac3000e09e061b4582f922fded6 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java cc6b53b3265253f76c1e954c0108aa5936f5cc36 src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 87203690f09456ac1ca5e9da2b82826d60cbd723 src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java aaedb3b5ec2cb27550449435efa8f335c6a9baad src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 12ea4c67350c2992f59bacd21a99d1413b60b757 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java 94f0a179b786649775899f855f7c1a0caab7290f src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java e113eba1f304279b5ee3d70db1d1ea558efd63ac src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java b60b004adbd6753ec6fef125fd70286be5071c56 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c Diff: https://reviews.apache.org/r/27705/diff/ Testing --- ./gradlew -Pq build Verified new stats in vagrant. Thanks, Maxim Khutornenko
Re: Review Request 28026: Add more test coverage to SchedulerThriftInterface.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28026/#review61387 --- This patch does not apply cleanly on master (316f291), do you need to rebase? I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 14, 2014, 1:30 a.m., Bill Farner wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28026/ --- (Updated Nov. 14, 2014, 1:30 a.m.) Review request for Aurora, David McLaughlin and Zameer Manji. Repository: aurora Description --- This brings SchedulerThriftInterface to 99% instruction coverage and 96% branch coverage. The remaining pieces are legitimately difficult to capture. I also uncovered a few minor bugs along the way, most noteworthy being AOP interceptor ordering leading to the standard API response not always being applied. Additionally, i removed a bunch of unnecessary exception handling that is now done by an AOP interceptor. Diffs - src/main/java/org/apache/aurora/auth/SessionValidator.java eeebb78901a6c33e08ceb8e675c91f0b5f44bcbc src/main/java/org/apache/aurora/scheduler/storage/backup/Recovery.java 4744dc9f202969906113ccb610bf17c94d188c43 src/main/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterface.java b2b66acee9c0789f3660469d6d504b4510af5e79 src/main/java/org/apache/aurora/scheduler/thrift/Util.java 18e2bdfab6406761d7e7cbcec26d12fb28441fe0 src/main/java/org/apache/aurora/scheduler/thrift/aop/AopModule.java dca855c522d21924821fc47e636da39689aec4b7 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c Diff: https://reviews.apache.org/r/28026/diff/ Testing --- Thanks, Bill Farner
Re: Review Request 28019: Show update page URL in the output for beta-update start.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/#review61386 --- Ship it! Master (316f291) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 14, 2014, 1:44 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28019/ --- (Updated Nov. 14, 2014, 1:44 a.m.) Review request for Aurora, Maxim Khutornenko and Bill Farner. Bugs: AURORA-766 https://issues.apache.org/jira/browse/AURORA-766 Repository: aurora Description --- Show update page URL in the output for beta-update start. Diffs - src/main/python/apache/aurora/client/cli/context.py df9dc24a060ef551009f48687fe9ecd928ee04a4 src/main/python/apache/aurora/client/cli/update.py 30763920bbc0b7e7a19e9a1a873d72680b7f821b src/test/python/apache/aurora/client/cli/test_supdate.py 97a9664a5230a7331bd7b784cbbc958d4c07cbb1 Diff: https://reviews.apache.org/r/28019/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli/:all ./build-support/python/checkstyle-check src/ ./build-support/python/isort-check src/ Thanks, David McLaughlin
Re: Review Request 27852: Ensure run verb returns an exit code.
On Nov. 11, 2014, 10:55 p.m., Bill Farner wrote: src/main/python/apache/aurora/client/cli/task.py, line 72 https://reviews.apache.org/r/27852/diff/1/?file=757426#file757426line72 Should we just make this the default behavior? There's at least 31 locations that do this, seems like a catch-all would be useful. If we do this, i'd recommend you remove all `return EXIT_OK` lines, so only proceed if you're okay with that. Looks like the relevant changes would be in: src/main/python/apache/aurora/client/cli/standalone_client.py src/main/python/apache/aurora/client/cli/client.py Zameer Manji wrote: I don't see how this approach will work. This problem of not returning an exit code comes from the subclasses not implementing the method correctly not that we dispatch to the super class's implementation. In addition I'm not comfortable mixing in this (minor) change with a larger structural change. Bill Farner wrote: Are we talking about different things? I'm just suggesting a change like this, around sys.exit: ``` client = AuroraCommandLine() if len(sys.argv) == 1: sys.argv.append(help) - sys.exit(client.execute(sys.argv[1:])) + exit_code = client.execute(sys.argv[1:]) + sys.exit(0 if exit_code is None else exit_code) if __name__ == '__main__': proxy_main() ``` This is basically just changing the contract to exit 0 if no explicit exit code is returned. Given we're working in a language that doesn't have a compiler to enforce a return value, i think this is the right thing to do. Zameer Manji wrote: The python philisohpy is 'explicit over implicit'. I feel that returning None is an error (that we should catch in our tests) and we should explicitly returning the exit code. Works for me, in that case, let's own up and change the code i cited above to assert not None. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/#review60888 --- On Nov. 11, 2014, 2:50 a.m., Zameer Manji wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27852/ --- (Updated Nov. 11, 2014, 2:50 a.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-923 https://issues.apache.org/jira/browse/AURORA-923 Repository: aurora Description --- Ensure run verb returns an exit code. Diffs - src/main/python/apache/aurora/client/cli/task.py 91175facdc8c9fd59ab66781f86ee8b5940a src/test/python/apache/aurora/client/cli/BUILD e1f9ebf96774b8f5c75de8570c6ba87d953ab649 src/test/python/apache/aurora/client/cli/test_task_run.py 8d9ef0543c1ab514d6f039ba63a1d417a4a90a1b Diff: https://reviews.apache.org/r/27852/diff/ Testing --- ./pants build src/test/python/apache/aurora/client/cli:: Thanks, Zameer Manji
Re: Review Request 27698: Move zookeeper connection off the main thread to prevent client deadlocks.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27698/#review61390 --- Ship it! Master (316f291) is green with this patch. ./build-support/jenkins/build.sh I will refresh this build result if you post a review containing @ReviewBot retry - Aurora ReviewBot On Nov. 14, 2014, 1:03 a.m., David McLaughlin wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27698/ --- (Updated Nov. 14, 2014, 1:03 a.m.) Review request for Aurora, Kevin Sweeney, Brian Wickman, and Zameer Manji. Repository: aurora Description --- The underlying Kazoo code here has an uninterruptable wait on the main thread, which means if there are any problems establishing a connection then you need to hard kill the client process. Moving the code to a separate thread. Diffs - src/main/python/apache/aurora/client/api/scheduler_client.py 3a851cc9f53380b4bf2e9df47080e7c476c3e38e src/test/python/apache/aurora/client/api/test_scheduler_client.py 1f1c6e0ab686cc63bf0a6b7a36e7eef688644e74 Diff: https://reviews.apache.org/r/27698/diff/ Testing --- ./pants src/test/python/apache/aurora/client:all src.test.python.apache.aurora.client.api.api . SUCCESS src.test.python.apache.aurora.client.api.disambiguator . SUCCESS src.test.python.apache.aurora.client.api.instance_watcher . SUCCESS src.test.python.apache.aurora.client.api.job_monitor . SUCCESS src.test.python.apache.aurora.client.api.mux . SUCCESS src.test.python.apache.aurora.client.api.quota_check . SUCCESS src.test.python.apache.aurora.client.api.restarter . SUCCESS src.test.python.apache.aurora.client.api.scheduler_client . SUCCESS src.test.python.apache.aurora.client.api.sla . SUCCESS src.test.python.apache.aurora.client.api.updater . SUCCESS src.test.python.apache.aurora.client.api.updater_util . SUCCESS src.test.python.apache.aurora.client.binding_helper . SUCCESS src.test.python.apache.aurora.client.cli.api . SUCCESS src.test.python.apache.aurora.client.cli.bridge . SUCCESS src.test.python.apache.aurora.client.cli.command_hooks . SUCCESS src.test.python.apache.aurora.client.cli.config . SUCCESS src.test.python.apache.aurora.client.cli.cron . SUCCESS src.test.python.apache.aurora.client.cli.help . SUCCESS src.test.python.apache.aurora.client.cli.inspect . SUCCESS src.test.python.apache.aurora.client.cli.job . SUCCESS src.test.python.apache.aurora.client.cli.logging . SUCCESS src.test.python.apache.aurora.client.cli.plugins . SUCCESS src.test.python.apache.aurora.client.cli.quota . SUCCESS src.test.python.apache.aurora.client.cli.sla . SUCCESS src.test.python.apache.aurora.client.cli.supdate . SUCCESS src.test.python.apache.aurora.client.cli.task . SUCCESS src.test.python.apache.aurora.client.cli.update . SUCCESS src.test.python.apache.aurora.client.commands.admin . SUCCESS src.test.python.apache.aurora.client.commands.core . SUCCESS src.test.python.apache.aurora.client.commands.hooks . SUCCESS src.test.python.apache.aurora.client.commands.maintenance . SUCCESS src.test.python.apache.aurora.client.commands.run . SUCCESS src.test.python.apache.aurora.client.commands.ssh . SUCCESS src.test.python.apache.aurora.client.config . SUCCESS src.test.python.apache.aurora.client.hooks.hooked_api . SUCCESS src.test.python.apache.aurora.client.hooks.non_hooked_api
Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.
On Nov. 14, 2014, 2:24 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/TaskVars.java, line 226 https://reviews.apache.org/r/27705/diff/2/?file=763034#file763034line226 To get the data we want, some extra analysis is needed. Specifically - if we want to figure out how often a scheduling attempt is vetoed _only_ for static reasons (e.g. insufficient resources), these stats will lack signal. Instead, we probably want two counters: - scheduling_veto_static - scheduling_veto_dynamic Does that make sense? I don't see how more granular data would prevent us from aggregating into static/dynamic groups. However, having aggregate metrics instead will make it impossible to do any further analysis when needed. Why not going the more specific route instead? I would have hard time figuring out what scheduling_veto_static means without digging through the sources, whereas something like scheduling_veto_INSUFFICIENT_RESOURCES would immediately make sense by itself. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/#review61385 --- On Nov. 14, 2014, 12:30 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27705/ --- (Updated Nov. 14, 2014, 12:30 a.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-914 https://issues.apache.org/jira/browse/AURORA-914 Repository: aurora Description --- Adding @Timed to trace scheduling latencies and Veto counters per type. Diffs - src/main/java/org/apache/aurora/scheduler/TaskVars.java cf8f7584afee758c527798914181049051aef0d8 src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java d2682cd910d248c897e691bcb4c8a3a6f1aec2d2 src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java e2ba8b8fe978a58d1edcd01963ea020e54529353 src/main/java/org/apache/aurora/scheduler/filter/ConstraintFilter.java 3839083f27ca5d4b93406152559b58b04e912a10 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java c1c5f26723f1eac3000e09e061b4582f922fded6 src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java cc6b53b3265253f76c1e954c0108aa5936f5cc36 src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java 87203690f09456ac1ca5e9da2b82826d60cbd723 src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java aaedb3b5ec2cb27550449435efa8f335c6a9baad src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java 12ea4c67350c2992f59bacd21a99d1413b60b757 src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java 94f0a179b786649775899f855f7c1a0caab7290f src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java e113eba1f304279b5ee3d70db1d1ea558efd63ac src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java b60b004adbd6753ec6fef125fd70286be5071c56 src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java 5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c Diff: https://reviews.apache.org/r/27705/diff/ Testing --- ./gradlew -Pq build Verified new stats in vagrant. Thanks, Maxim Khutornenko
Re: Review Request 27705: Adding instrumentation into the scheduling pipeline.
On Nov. 14, 2014, 2:24 a.m., Bill Farner wrote:
src/main/java/org/apache/aurora/scheduler/TaskVars.java, line 226
https://reviews.apache.org/r/27705/diff/2/?file=763034#file763034line226
To get the data we want, some extra analysis is needed. Specifically -
if we want to figure out how often a scheduling attempt is vetoed _only_
for static reasons (e.g. insufficient resources), these stats will lack
signal.
Instead, we probably want two counters:
- scheduling_veto_static
- scheduling_veto_dynamic
Does that make sense?
Maxim Khutornenko wrote:
I don't see how more granular data would prevent us from aggregating into
static/dynamic groups. However, having aggregate metrics instead will make it
impossible to do any further analysis when needed. Why not going the more
specific route instead? I would have hard time figuring out what
scheduling_veto_static means without digging through the sources, whereas
something like scheduling_veto_INSUFFICIENT_RESOURCES would immediately
make sense by itself.
The problem is that you can't discern when a task didn't match due to _only_
static reasons. Relevant code in `SchedulingFilterImpl`:
return ImmutableSet.Vetobuilder()
.addAll(getConstraintFilter(attributeAggregate, attributes).apply(task))
.addAll(getResourceVetoes(offer, task))
.build();
On the other end when you incrmeent counters:
for (Veto veto : event.getVetoes()) {
counters.getUnchecked(vetoStatName(veto)).increment();
}
At this point, you might get vetoes like: `insufficient CPU`, `insufficient
RAM`, `insufficient ports`, `limit not satisfied: host`.
You'll end up with these counter deltas:
`INSUFFICIENT_RESOURCES 3`
`LIMIT_NOT_SATISFIED 1`
As a result, i don't see how we could look at the stats and convince ourselves
which optimization has the greatest payoff, since a single scheduling round
affects multiple counters disproportionately.
- Bill
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27705/#review61385
---
On Nov. 14, 2014, 12:30 a.m., Maxim Khutornenko wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27705/
---
(Updated Nov. 14, 2014, 12:30 a.m.)
Review request for Aurora, Bill Farner and Zameer Manji.
Bugs: AURORA-914
https://issues.apache.org/jira/browse/AURORA-914
Repository: aurora
Description
---
Adding @Timed to trace scheduling latencies and Veto counters per type.
Diffs
-
src/main/java/org/apache/aurora/scheduler/TaskVars.java
cf8f7584afee758c527798914181049051aef0d8
src/main/java/org/apache/aurora/scheduler/async/OfferQueue.java
d2682cd910d248c897e691bcb4c8a3a6f1aec2d2
src/main/java/org/apache/aurora/scheduler/async/TaskScheduler.java
e2ba8b8fe978a58d1edcd01963ea020e54529353
src/main/java/org/apache/aurora/scheduler/filter/ConstraintFilter.java
3839083f27ca5d4b93406152559b58b04e912a10
src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilter.java
c1c5f26723f1eac3000e09e061b4582f922fded6
src/main/java/org/apache/aurora/scheduler/filter/SchedulingFilterImpl.java
cc6b53b3265253f76c1e954c0108aa5936f5cc36
src/main/java/org/apache/aurora/scheduler/metadata/NearestFit.java
87203690f09456ac1ca5e9da2b82826d60cbd723
src/main/java/org/apache/aurora/scheduler/stats/CachedCounters.java
aaedb3b5ec2cb27550449435efa8f335c6a9baad
src/test/java/org/apache/aurora/scheduler/TaskVarsTest.java
12ea4c67350c2992f59bacd21a99d1413b60b757
src/test/java/org/apache/aurora/scheduler/events/NotifyingSchedulingFilterTest.java
94f0a179b786649775899f855f7c1a0caab7290f
src/test/java/org/apache/aurora/scheduler/filter/SchedulingFilterImplTest.java
e113eba1f304279b5ee3d70db1d1ea558efd63ac
src/test/java/org/apache/aurora/scheduler/metadata/NearestFitTest.java
b60b004adbd6753ec6fef125fd70286be5071c56
src/test/java/org/apache/aurora/scheduler/thrift/SchedulerThriftInterfaceTest.java
5c9ea6cf4eb4d99d94f5d61e784dd7c9c480798c
Diff: https://reviews.apache.org/r/27705/diff/
Testing
---
./gradlew -Pq build
Verified new stats in vagrant.
Thanks,
Maxim Khutornenko
