You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Mark Chu-Carroll <mc...@twopensource.com> on 2014/07/01 17:02:02 UTC
Review Request 23199: Log loaded config file at level TRANSCRIPT (aka INFO+1)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/
-----------------------------------------------------------
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Repository: aurora
Description
-------
Have client commands that load config files print the contents of the loaded config file to
log at level TRANSCRIPT (aka INFO+1).
Diffs
-----
src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
Diff: https://reviews.apache.org/r/23199/diff/
Testing
-------
Added unit test of new functionality, ran all tests.
Thanks,
Mark Chu-Carroll
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/#review47073
-----------------------------------------------------------
Ship it!
Ship It!
- Maxim Khutornenko
On July 1, 2014, 3:03 p.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23199/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 3:03 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: aurora-567
> https://issues.apache.org/jira/browse/aurora-567
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Have client commands that load config files print the contents of the loaded config file to
> log at level TRANSCRIPT (aka INFO+1).
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
> src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
>
> Diff: https://reviews.apache.org/r/23199/diff/
>
>
> Testing
> -------
>
> Added unit test of new functionality, ran all tests.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/#review47070
-----------------------------------------------------------
src/main/python/apache/aurora/client/cli/context.py
<https://reviews.apache.org/r/23199/#comment82672>
Don't you want to except IOError here in case the file does not exist or cannot be read?
- Maxim Khutornenko
On July 1, 2014, 3:03 p.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23199/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 3:03 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: aurora-567
> https://issues.apache.org/jira/browse/aurora-567
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Have client commands that load config files print the contents of the loaded config file to
> log at level TRANSCRIPT (aka INFO+1).
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
> src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
>
> Diff: https://reviews.apache.org/r/23199/diff/
>
>
> Testing
> -------
>
> Added unit test of new functionality, ran all tests.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/#review47071
-----------------------------------------------------------
src/main/python/apache/aurora/client/cli/context.py
<https://reviews.apache.org/r/23199/#comment82673>
Nope. It'll get caught by the general catch below, which generates an appropriate error message.
- Mark Chu-Carroll
On July 1, 2014, 11:03 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23199/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 11:03 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: aurora-567
> https://issues.apache.org/jira/browse/aurora-567
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Have client commands that load config files print the contents of the loaded config file to
> log at level TRANSCRIPT (aka INFO+1).
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
> src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
>
> Diff: https://reviews.apache.org/r/23199/diff/
>
>
> Testing
> -------
>
> Added unit test of new functionality, ran all tests.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/#review47137
-----------------------------------------------------------
Ship it!
Ship It!
- Kevin Sweeney
On July 1, 2014, 1:18 p.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23199/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 1:18 p.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: aurora-567
> https://issues.apache.org/jira/browse/aurora-567
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Have client commands that load config files print the contents of the loaded config file to
> log at level TRANSCRIPT (aka INFO+1).
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
> src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
>
> Diff: https://reviews.apache.org/r/23199/diff/
>
>
> Testing
> -------
>
> Added unit test of new functionality, ran all tests.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/
-----------------------------------------------------------
(Updated July 1, 2014, 4:18 p.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Changes
-------
push the transcript loglevel around.
Bugs: aurora-567
https://issues.apache.org/jira/browse/aurora-567
Repository: aurora
Description
-------
Have client commands that load config files print the contents of the loaded config file to
log at level TRANSCRIPT (aka INFO+1).
Diffs (updated)
-----
src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
Diff: https://reviews.apache.org/r/23199/diff/
Testing
-------
Added unit test of new functionality, ran all tests.
Thanks,
Mark Chu-Carroll
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Kevin Sweeney <ke...@apache.org>.
> On July 1, 2014, 10:55 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/cli/context.py, lines 63-64
> > <https://reviews.apache.org/r/23199/diff/1/?file=621349#file621349line63>
> >
> > This doesn't play nicely with the include() directive, which may load other config files from elsewhere. It also introduces a race (you open and close the config file here, then the loader presumably does as well meaning that the file contents could change after you've logged them). I wonder if this logic would be better pushed down to the loader.
>
> Mark Chu-Carroll wrote:
> (1) Race condition really isn't an issue here. If a user is modifying a config file while aurora is running, the results will (at best) be flaky. This doesn't make that any worse.
>
> (2) I'd like to put it in the loader, but it's not feasible. The loader is, ultimately, inside of pystachio (via a complex route: get_config -> AnnotatedAuroraConfig.load -> AuroraConfig.load -> AuroraConfigLoader.load -> AuroraConfigLoader.load_raw -> PystachioConfig.__apply_)
>
> (3) This doesn't follow include semantics - but it's not trying to solve that problem. The way that the aurora docs read, the config file should be at the path specified on the command-line. If it's not, it should be an error. For this level of logging, I'm not even trying to resolve includes - just simply dumping file contents.
>
Sounds reasonable, maybe a TODO to send a pull request (add a logger to the pystachio library that we can configure in our application initialization code).
> On July 1, 2014, 10:55 a.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 64
> > <https://reviews.apache.org/r/23199/diff/1/?file=621349#file621349line64>
> >
> > Add a constant for the TRANSCRIPT loglevel somewhere?
>
> Mark Chu-Carroll wrote:
> There is, but using it here would create a circular dependency.
>
Can you refactor it into an interface file to remove the circular dependency? Copy-pasting the constant doesn't actually remove the circular dep, just makes it implicit, and the DRY violation makes the code very brittle to refactoring in the future.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/#review47106
-----------------------------------------------------------
On July 1, 2014, 8:03 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23199/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 8:03 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: aurora-567
> https://issues.apache.org/jira/browse/aurora-567
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Have client commands that load config files print the contents of the loaded config file to
> log at level TRANSCRIPT (aka INFO+1).
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
> src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
>
> Diff: https://reviews.apache.org/r/23199/diff/
>
>
> Testing
> -------
>
> Added unit test of new functionality, ran all tests.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
> On July 1, 2014, 1:55 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/cli/context.py, lines 63-64
> > <https://reviews.apache.org/r/23199/diff/1/?file=621349#file621349line63>
> >
> > This doesn't play nicely with the include() directive, which may load other config files from elsewhere. It also introduces a race (you open and close the config file here, then the loader presumably does as well meaning that the file contents could change after you've logged them). I wonder if this logic would be better pushed down to the loader.
(1) Race condition really isn't an issue here. If a user is modifying a config file while aurora is running, the results will (at best) be flaky. This doesn't make that any worse.
(2) I'd like to put it in the loader, but it's not feasible. The loader is, ultimately, inside of pystachio (via a complex route: get_config -> AnnotatedAuroraConfig.load -> AuroraConfig.load -> AuroraConfigLoader.load -> AuroraConfigLoader.load_raw -> PystachioConfig.__apply_)
(3) This doesn't follow include semantics - but it's not trying to solve that problem. The way that the aurora docs read, the config file should be at the path specified on the command-line. If it's not, it should be an error. For this level of logging, I'm not even trying to resolve includes - just simply dumping file contents.
> On July 1, 2014, 1:55 p.m., Kevin Sweeney wrote:
> > src/main/python/apache/aurora/client/cli/context.py, line 64
> > <https://reviews.apache.org/r/23199/diff/1/?file=621349#file621349line64>
> >
> > Add a constant for the TRANSCRIPT loglevel somewhere?
There is, but using it here would create a circular dependency.
- Mark
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/#review47106
-----------------------------------------------------------
On July 1, 2014, 11:03 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23199/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 11:03 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: aurora-567
> https://issues.apache.org/jira/browse/aurora-567
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Have client commands that load config files print the contents of the loaded config file to
> log at level TRANSCRIPT (aka INFO+1).
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
> src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
>
> Diff: https://reviews.apache.org/r/23199/diff/
>
>
> Testing
> -------
>
> Added unit test of new functionality, ran all tests.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/#review47106
-----------------------------------------------------------
src/main/python/apache/aurora/client/cli/context.py
<https://reviews.apache.org/r/23199/#comment82712>
This doesn't play nicely with the include() directive, which may load other config files from elsewhere. It also introduces a race (you open and close the config file here, then the loader presumably does as well meaning that the file contents could change after you've logged them). I wonder if this logic would be better pushed down to the loader.
src/main/python/apache/aurora/client/cli/context.py
<https://reviews.apache.org/r/23199/#comment82711>
Add a constant for the TRANSCRIPT loglevel somewhere?
- Kevin Sweeney
On July 1, 2014, 8:03 a.m., Mark Chu-Carroll wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/23199/
> -----------------------------------------------------------
>
> (Updated July 1, 2014, 8:03 a.m.)
>
>
> Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
>
>
> Bugs: aurora-567
> https://issues.apache.org/jira/browse/aurora-567
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Have client commands that load config files print the contents of the loaded config file to
> log at level TRANSCRIPT (aka INFO+1).
>
>
> Diffs
> -----
>
> src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
> src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
>
> Diff: https://reviews.apache.org/r/23199/diff/
>
>
> Testing
> -------
>
> Added unit test of new functionality, ran all tests.
>
>
> Thanks,
>
> Mark Chu-Carroll
>
>
Re: Review Request 23199: Log loaded config file at level TRANSCRIPT (aka
INFO+1)
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/23199/
-----------------------------------------------------------
(Updated July 1, 2014, 11:03 a.m.)
Review request for Aurora, Kevin Sweeney and Maxim Khutornenko.
Bugs: aurora-567
https://issues.apache.org/jira/browse/aurora-567
Repository: aurora
Description
-------
Have client commands that load config files print the contents of the loaded config file to
log at level TRANSCRIPT (aka INFO+1).
Diffs
-----
src/main/python/apache/aurora/client/cli/context.py facd52c14e330c35889cec0292bb380cecff9641
src/test/python/apache/aurora/client/cli/test_logging.py b3c3b8deaa8961251a1be8121659e742b00f6df2
Diff: https://reviews.apache.org/r/23199/diff/
Testing
-------
Added unit test of new functionality, ran all tests.
Thanks,
Mark Chu-Carroll