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