aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Zameer Manji" <zma...@twopensource.com>
Subject Re: Review Request 28876: Suppressing redundant client command error messaging.
Date Wed, 10 Dec 2014 19:37:42 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message