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

Reply via email to