You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/12/09 23:32:57 UTC
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/
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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
>
>
Re: Review Request 28876: Suppressing redundant client command error
messaging.
Posted by Bill Farner <wf...@apache.org>.
> 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.
Posted by Zameer Manji <zm...@twopensource.com>.
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
Posted by Bill Farner <wf...@apache.org>.
> 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.
Posted by Zameer Manji <zm...@twopensource.com>.
> 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.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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,
>
> Maxim Khutornenko
>
>
Re: Review Request 28876: Suppressing redundant client command error
messaging.
Posted by Maxim Khutornenko <ma...@apache.org>.
> 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.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
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.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28876/#review64791
-----------------------------------------------------------
Ship it!
Ship It!
- Bill Farner
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.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
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
>
>