Re: Review Request 32014: Adding more logging into MaintenanceController.
On March 13, 2015, 2:30 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java, line 255 https://reviews.apache.org/r/32014/diff/1/?file=892576#file892576line255 This is a pretty weird log entry to have. Can it be done in the client instead? Maxim Khutornenko wrote: Client already has logs but that's not enough to reconstruct the behavior when maintenance stalls. We need to see both sides of the story to properly troubleshoot host draining issues. Bill Farner wrote: This one i still don't follow - AFAICT it's literally printing the response handed directly to the client, which is the only caller if i'm reading correctly. Maxim Khutornenko wrote: Having it only on the client hampers investigation when client logs are missing or incomplete. This logging completes the scheduler view of the maintenance story, which can be reconstructed any time we have a question and the client logs a long gone. Chatted with Bill offline. While it's certainly tempting to add a response logging in the scheduler, it does not align with our general approach not to. Perhaps we have a better answer with AURORA-189. I will drop this particular statement and add a more verbose status logging on the client instead. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/#review76332 --- On March 13, 2015, 1:23 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 1:23 a.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Also added missing test coverage. Diffs - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 32014: Adding more logging into MaintenanceController.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 5:59 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1170 https://issues.apache.org/jira/browse/AURORA-1170 Repository: aurora Description --- Also added missing test coverage. Diffs - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 32014: Adding more logging into MaintenanceController.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/#review76419 --- Ship it! Master (128e554) 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 March 13, 2015, 8:06 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 8:06 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1170 https://issues.apache.org/jira/browse/AURORA-1170 Repository: aurora Description --- Also added missing test coverage. Diffs - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/main/python/apache/aurora/admin/host_maintenance.py 8fa5182fae509493def7175635fbe1cb79fa3eec src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 src/test/python/apache/aurora/admin/test_host_maintenance.py bb586700814a96b3e83d11728b462a7765e81bc1 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 32014: Adding more logging into MaintenanceController.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 8:05 p.m.) Review request for Aurora and Bill Farner. Changes --- Bill's comments. Bugs: AURORA-1170 https://issues.apache.org/jira/browse/AURORA-1170 Repository: aurora Description --- Also added missing test coverage. Diffs (updated) - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/main/python/apache/aurora/admin/host_maintenance.py 8fa5182fae509493def7175635fbe1cb79fa3eec src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 src/test/python/apache/aurora/admin/test_host_maintenance.py bb586700814a96b3e83d11728b462a7765e81bc1 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 32014: Adding more logging into MaintenanceController.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 8:06 p.m.) Review request for Aurora and Bill Farner. Bugs: AURORA-1170 https://issues.apache.org/jira/browse/AURORA-1170 Repository: aurora Description --- Also added missing test coverage. Diffs (updated) - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/main/python/apache/aurora/admin/host_maintenance.py 8fa5182fae509493def7175635fbe1cb79fa3eec src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 src/test/python/apache/aurora/admin/test_host_maintenance.py bb586700814a96b3e83d11728b462a7765e81bc1 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 32014: Adding more logging into MaintenanceController.
On March 13, 2015, 2:30 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java, line 279 https://reviews.apache.org/r/32014/diff/1/?file=892576#file892576line279 Ditto - this really seems like it should be client-side logging. The client gets back the actiosn that were taken, seems like it has all the info it needs to check for discrepancies. Maxim Khutornenko wrote: This is particularly important for troubleshooting as we have occasionally observed abnormal behavior that possibly points to host attributes handling. Lack of logs around that does not help us there. Given the relatively infrequent use of this functionality, I'd rather have more data points here. Aha, i was operating on the incorrect assumption that this plumbed directly out to the thift interface, but it has internal callers as well. On March 13, 2015, 2:30 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java, line 255 https://reviews.apache.org/r/32014/diff/1/?file=892576#file892576line255 This is a pretty weird log entry to have. Can it be done in the client instead? Maxim Khutornenko wrote: Client already has logs but that's not enough to reconstruct the behavior when maintenance stalls. We need to see both sides of the story to properly troubleshoot host draining issues. This one i still don't follow - AFAICT it's literally printing the response handed directly to the client, which is the only caller if i'm reading correctly. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/#review76332 --- On March 13, 2015, 1:23 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 1:23 a.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Also added missing test coverage. Diffs - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 32014: Adding more logging into MaintenanceController.
On March 13, 2015, 2:30 a.m., Bill Farner wrote: src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java, line 255 https://reviews.apache.org/r/32014/diff/1/?file=892576#file892576line255 This is a pretty weird log entry to have. Can it be done in the client instead? Maxim Khutornenko wrote: Client already has logs but that's not enough to reconstruct the behavior when maintenance stalls. We need to see both sides of the story to properly troubleshoot host draining issues. Bill Farner wrote: This one i still don't follow - AFAICT it's literally printing the response handed directly to the client, which is the only caller if i'm reading correctly. Having it only on the client hampers investigation when client logs are missing or incomplete. This logging completes the scheduler view of the maintenance story, which can be reconstructed any time we have a question and the client logs a long gone. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/#review76332 --- On March 13, 2015, 1:23 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 1:23 a.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Also added missing test coverage. Diffs - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 32014: Adding more logging into MaintenanceController.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/#review76332 --- src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java https://reviews.apache.org/r/32014/#comment123873 `Tasks.ids(activeTasks)` src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java https://reviews.apache.org/r/32014/#comment123874 This is a pretty weird log entry to have. Can it be done in the client instead? src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java https://reviews.apache.org/r/32014/#comment123875 Ditto - this really seems like it should be client-side logging. The client gets back the actiosn that were taken, seems like it has all the info it needs to check for discrepancies. - Bill Farner On March 13, 2015, 1:23 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 1:23 a.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Also added missing test coverage. Diffs - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Review Request 32014: Adding more logging into MaintenanceController.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- Review request for Aurora and Bill Farner. Repository: aurora Description --- Also added missing test coverage. Diffs - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
Re: Review Request 32014: Adding more logging into MaintenanceController.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/#review76327 --- Ship it! Master (ee1a13a) 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 March 13, 2015, 1:23 a.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32014/ --- (Updated March 13, 2015, 1:23 a.m.) Review request for Aurora and Bill Farner. Repository: aurora Description --- Also added missing test coverage. Diffs - config/legacy_untested_classes.txt 06e130fae6bdc43b5e8aff182a530eac65acca53 src/main/java/org/apache/aurora/scheduler/state/MaintenanceController.java b6f642e319b790544bc538098e7cb78b1bb49997 src/test/java/org/apache/aurora/scheduler/state/MaintenanceControllerImplTest.java bd031a5e0b7d2923e36ca5958c5074f40dc64848 Diff: https://reviews.apache.org/r/32014/diff/ Testing --- ./gradlew -Pq build Thanks, Maxim Khutornenko
