Re: Review Request 28876: Suppressing redundant client command error messaging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64787 --- Ping, Bill. - Maxim Khutornenko On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 10:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
On Dec. 9, 2014, 3:29 p.m., Zameer Manji wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 384 https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384 This solution seems to be papering over the problem we have with output in the client. Instead of creating a new type of exception can we just prevent the double printing in general? Maxim Khutornenko wrote: Not sure I buy it. This `print_err()` is needed when a CommandError is raised to bail out due to some internal problem (i.e. not related to scheduler call and not routed through context.check_and_log_response). The fact that the same error type is used for both is exactly the reason we have these redundant messages. There are plenty of legitimate cases where the error needs to be logged. Consider this example: ``` $ aurora2 job kill devcluster/www-data/prod/hello Error executing command: The instances list cannot be omitted in a kill command!; use killall to kill all instances ``` Bill Farner wrote: Part of the problem here is that logging to stdout/stderr happens throughout the stack. This seems to inevitably lead towards redundant output. Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? The only use case that seems to not fit that today seems to be client-side job updates, which we could ignore since it's scheduled for removal. Maxim Khutornenko wrote: | Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? Agree, that would make sense. There are plenty of examples where that happens. E.g. compare [1] and [2]. The first one has redundant print_err() and will result in duplicate messages. The second one (used as example above) only loggged by the CommineLine handler. This diff addresses a problem where a redundant incomplete message is logged after unsuccessful scheduler call. I suggest we tackle the [1] separately. [1] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L342-L344 [2] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L368-L370 Bill Farner wrote: Going back to Zameer's comment, though, maybe `check_and_log_response` should refrain from calling `print_err`? Seems like the receiving end of `CommandError` should be responsible for presenting the error. Maxim Khutornenko wrote: I am not sure what it really buys us. Right now, the scheduler response is entirely handled within the check_and_log_response that does centralized logging to both stdout and stderr. IMO, delegating just stderr handling elsewhere creates more ambiguity and is not solving any additional problems. Maxim, if you agree to tackle the redundant print_err()/CommandError in another RB I am comfortable with this change. - Zameer --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- On Dec. 9, 2014, 2:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 2:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 384 https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384 This solution seems to be papering over the problem we have with output in the client. Instead of creating a new type of exception can we just prevent the double printing in general? Maxim Khutornenko wrote: Not sure I buy it. This `print_err()` is needed when a CommandError is raised to bail out due to some internal problem (i.e. not related to scheduler call and not routed through context.check_and_log_response). The fact that the same error type is used for both is exactly the reason we have these redundant messages. There are plenty of legitimate cases where the error needs to be logged. Consider this example: ``` $ aurora2 job kill devcluster/www-data/prod/hello Error executing command: The instances list cannot be omitted in a kill command!; use killall to kill all instances ``` Bill Farner wrote: Part of the problem here is that logging to stdout/stderr happens throughout the stack. This seems to inevitably lead towards redundant output. Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? The only use case that seems to not fit that today seems to be client-side job updates, which we could ignore since it's scheduled for removal. Maxim Khutornenko wrote: | Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? Agree, that would make sense. There are plenty of examples where that happens. E.g. compare [1] and [2]. The first one has redundant print_err() and will result in duplicate messages. The second one (used as example above) only loggged by the CommineLine handler. This diff addresses a problem where a redundant incomplete message is logged after unsuccessful scheduler call. I suggest we tackle the [1] separately. [1] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L342-L344 [2] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L368-L370 Bill Farner wrote: Going back to Zameer's comment, though, maybe `check_and_log_response` should refrain from calling `print_err`? Seems like the receiving end of `CommandError` should be responsible for presenting the error. Maxim Khutornenko wrote: I am not sure what it really buys us. Right now, the scheduler response is entirely handled within the check_and_log_response that does centralized logging to both stdout and stderr. IMO, delegating just stderr handling elsewhere creates more ambiguity and is not solving any additional problems. Zameer Manji wrote: Maxim, if you agree to tackle the redundant print_err()/CommandError in another RB I am comfortable with this change. These are two different issues and I was going to address the other one in a separate RB. Filed AURORA-968 to track it. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 10:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks,
Re: Review Request 28876: Suppressing redundant client command error messaging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64677 --- Ship it! Ship It! - Zameer Manji On Dec. 9, 2014, 2:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 2:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
On Dec. 9, 2014, 3:29 p.m., Zameer Manji wrote: src/test/python/apache/aurora/client/cli/test_update.py, line 69 https://reviews.apache.org/r/28876/diff/1/?file=787345#file787345line69 Is it at all possible to avoid the use of raw Mock objects? Maxim Khutornenko wrote: This goes into Pystachio namespace that I am hesitant to tap into. Given the additional complexity I don't see much value in creating a concrete object for a parent-mocked container. Fair enough. - Zameer --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- On Dec. 9, 2014, 2:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 2:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64453 --- Ship it! Master (9926af3) 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 Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 10:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- src/main/python/apache/aurora/client/cli/__init__.py https://reviews.apache.org/r/28876/#comment107180 This solution seems to be papering over the problem we have with output in the client. Instead of creating a new type of exception can we just prevent the double printing in general? src/test/python/apache/aurora/client/cli/test_update.py https://reviews.apache.org/r/28876/#comment107179 Is it at all possible to avoid the use of raw Mock objects? - Zameer Manji On Dec. 9, 2014, 2:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 2:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 384 https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384 This solution seems to be papering over the problem we have with output in the client. Instead of creating a new type of exception can we just prevent the double printing in general? Not sure I buy it. This `print_err()` is needed when a CommandError is raised to bail out due to some internal problem (i.e. not related to scheduler call and not routed through context.check_and_log_response). The fact that the same error type is used for both is exactly the reason we have these redundant messages. There are plenty of legitimate cases where the error needs to be logged. Consider this example: ``` $ aurora2 job kill devcluster/www-data/prod/hello Error executing command: The instances list cannot be omitted in a kill command!; use killall to kill all instances ``` On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote: src/test/python/apache/aurora/client/cli/test_update.py, line 69 https://reviews.apache.org/r/28876/diff/1/?file=787345#file787345line69 Is it at all possible to avoid the use of raw Mock objects? This goes into Pystachio namespace that I am hesitant to tap into. Given the additional complexity I don't see much value in creating a concrete object for a parent-mocked container. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 10:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 384 https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384 This solution seems to be papering over the problem we have with output in the client. Instead of creating a new type of exception can we just prevent the double printing in general? Maxim Khutornenko wrote: Not sure I buy it. This `print_err()` is needed when a CommandError is raised to bail out due to some internal problem (i.e. not related to scheduler call and not routed through context.check_and_log_response). The fact that the same error type is used for both is exactly the reason we have these redundant messages. There are plenty of legitimate cases where the error needs to be logged. Consider this example: ``` $ aurora2 job kill devcluster/www-data/prod/hello Error executing command: The instances list cannot be omitted in a kill command!; use killall to kill all instances ``` Part of the problem here is that logging to stdout/stderr happens throughout the stack. This seems to inevitably lead towards redundant output. Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? The only use case that seems to not fit that today seems to be client-side job updates, which we could ignore since it's scheduled for removal. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 10:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 384 https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384 This solution seems to be papering over the problem we have with output in the client. Instead of creating a new type of exception can we just prevent the double printing in general? Maxim Khutornenko wrote: Not sure I buy it. This `print_err()` is needed when a CommandError is raised to bail out due to some internal problem (i.e. not related to scheduler call and not routed through context.check_and_log_response). The fact that the same error type is used for both is exactly the reason we have these redundant messages. There are plenty of legitimate cases where the error needs to be logged. Consider this example: ``` $ aurora2 job kill devcluster/www-data/prod/hello Error executing command: The instances list cannot be omitted in a kill command!; use killall to kill all instances ``` Bill Farner wrote: Part of the problem here is that logging to stdout/stderr happens throughout the stack. This seems to inevitably lead towards redundant output. Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? The only use case that seems to not fit that today seems to be client-side job updates, which we could ignore since it's scheduled for removal. | Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? Agree, that would make sense. There are plenty of examples where that happens. E.g. compare [1] and [2]. The first one has redundant print_err() and will result in duplicate messages. The second one (used as example above) only loggged by the CommineLine handler. This diff addresses a problem where a redundant incomplete message is logged after unsuccessful scheduler call. I suggest we tackle the [1] separately. [1] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L342-L344 [2] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L368-L370 - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 10:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 384 https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384 This solution seems to be papering over the problem we have with output in the client. Instead of creating a new type of exception can we just prevent the double printing in general? Maxim Khutornenko wrote: Not sure I buy it. This `print_err()` is needed when a CommandError is raised to bail out due to some internal problem (i.e. not related to scheduler call and not routed through context.check_and_log_response). The fact that the same error type is used for both is exactly the reason we have these redundant messages. There are plenty of legitimate cases where the error needs to be logged. Consider this example: ``` $ aurora2 job kill devcluster/www-data/prod/hello Error executing command: The instances list cannot be omitted in a kill command!; use killall to kill all instances ``` Bill Farner wrote: Part of the problem here is that logging to stdout/stderr happens throughout the stack. This seems to inevitably lead towards redundant output. Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? The only use case that seems to not fit that today seems to be client-side job updates, which we could ignore since it's scheduled for removal. Maxim Khutornenko wrote: | Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? Agree, that would make sense. There are plenty of examples where that happens. E.g. compare [1] and [2]. The first one has redundant print_err() and will result in duplicate messages. The second one (used as example above) only loggged by the CommineLine handler. This diff addresses a problem where a redundant incomplete message is logged after unsuccessful scheduler call. I suggest we tackle the [1] separately. [1] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L342-L344 [2] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L368-L370 Going back to Zameer's comment, though, maybe `check_and_log_response` should refrain from calling `print_err`? Seems like the receiving end of `CommandError` should be responsible for presenting the error. - Bill --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 10:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
Re: Review Request 28876: Suppressing redundant client command error messaging.
On Dec. 9, 2014, 11:29 p.m., Zameer Manji wrote: src/main/python/apache/aurora/client/cli/__init__.py, line 384 https://reviews.apache.org/r/28876/diff/1/?file=787343#file787343line384 This solution seems to be papering over the problem we have with output in the client. Instead of creating a new type of exception can we just prevent the double printing in general? Maxim Khutornenko wrote: Not sure I buy it. This `print_err()` is needed when a CommandError is raised to bail out due to some internal problem (i.e. not related to scheduler call and not routed through context.check_and_log_response). The fact that the same error type is used for both is exactly the reason we have these redundant messages. There are plenty of legitimate cases where the error needs to be logged. Consider this example: ``` $ aurora2 job kill devcluster/www-data/prod/hello Error executing command: The instances list cannot be omitted in a kill command!; use killall to kill all instances ``` Bill Farner wrote: Part of the problem here is that logging to stdout/stderr happens throughout the stack. This seems to inevitably lead towards redundant output. Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? The only use case that seems to not fit that today seems to be client-side job updates, which we could ignore since it's scheduled for removal. Maxim Khutornenko wrote: | Perhaps we should take the approach of limiting use of stdout/stderr to the top of the stack whenever possible? Agree, that would make sense. There are plenty of examples where that happens. E.g. compare [1] and [2]. The first one has redundant print_err() and will result in duplicate messages. The second one (used as example above) only loggged by the CommineLine handler. This diff addresses a problem where a redundant incomplete message is logged after unsuccessful scheduler call. I suggest we tackle the [1] separately. [1] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L342-L344 [2] - https://github.com/apache/incubator-aurora/blob/master/src/main/python/apache/aurora/client/cli/jobs.py#L368-L370 Bill Farner wrote: Going back to Zameer's comment, though, maybe `check_and_log_response` should refrain from calling `print_err`? Seems like the receiving end of `CommandError` should be responsible for presenting the error. I am not sure what it really buys us. Right now, the scheduler response is entirely handled within the check_and_log_response that does centralized logging to both stdout and stderr. IMO, delegating just stderr handling elsewhere creates more ambiguity and is not solving any additional problems. - Maxim --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/#review64460 --- On Dec. 9, 2014, 10:32 p.m., Maxim Khutornenko wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28876/ --- (Updated Dec. 9, 2014, 10:32 p.m.) Review request for Aurora, Bill Farner and Zameer Manji. Bugs: AURORA-965 https://issues.apache.org/jira/browse/AURORA-965 Repository: aurora Description --- Added a new error type to raise when errors are logged explicitly. Diffs - src/main/python/apache/aurora/client/cli/__init__.py c2eb89cae536838ac4dd46b0125a248d84f6e54c src/main/python/apache/aurora/client/cli/context.py ad27eb5674fb85c5d3a26a014349abdea4fa2d64 src/test/python/apache/aurora/client/cli/test_update.py 1f061a39279bf5ef9d4e7279016bf07e164014bb Diff: https://reviews.apache.org/r/28876/diff/ Testing --- ./pants src/test/python/apache/aurora/client/cli:all Vagrant before: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello Error executing command: Job creation failed due to error: ``` Vagrant after: ``` $ aurora2 job create devcluster/www-data/prod/hello aurora/examples/jobs/hello_world.aurora INFO] Creating job hello Job creation failed due to error: Job already exists: www-data/prod/hello ``` Thanks, Maxim Khutornenko
