Seems like a good consensus to go private. Unless any other objections, this seems decided.
On Tue, Mar 3, 2015 at 10:50 AM, Chris Westin <[email protected]> wrote: > I've been making them private as I go -- that's how this came up, form a > code review. > > On Mon, Mar 2, 2015 at 8:49 PM, Hanifi Gunes <[email protected]> wrote: > > > Agreed. > > > > - Does anyone object to making all the loggers in the Drill codebase > > private > > from now on? > > Does this include making existing ones private as well? It would be nice > if > > those who are touching a piece of code make non-private loggers private. > > > > > > -Hanifi > > > > On Mon, Mar 2, 2015 at 8:03 PM, Steven Phillips <[email protected]> > > wrote: > > > > > I have no objection to making the loggers private, and your reasons > make > > > sense to me. But I do wonder if there was a reason for not making them > > > private to begin with. > > > > > > On Mon, Mar 2, 2015 at 12:27 PM, Aditya <[email protected]> wrote: > > > > > > > Sounds good to me. > > > > > > > > On Mon, Mar 2, 2015 at 11:28 AM, Chris Westin < > [email protected] > > > > > > > wrote: > > > > > > > > > I've noticed in the Drill codebase that loggers are being declared > > > static > > > > > final, but not private. Based on my past experience, this doesn't > > match > > > > > common > > > > > practice. > > > > > > > > > > Loggers should be private: > > > > > > > > > > (1) It's common practice. I've never seen otherwise in the past. A > > > quick > > > > > search turned up these examples in the top search results: > > > > > - http://www.vogella.com/tutorials/Logging/article.html > > > > > - > > > > > > > > > > > > > > > > > > > > http://examples.javacodegeeks.com/core-java/util/logging/java-util-logging-example/ > > > > > > > > > > (2) It prevents accidentally using the logger in a derived class. > > I've > > > > been > > > > > sent off to the wrong place a few times when looking at logs for > the > > > > Drill > > > > > code > > > > > base, because some derived classes have (accidentally?) used their > > > > > super-classes' loggers. Since the loggers aren't private, this > isn't > > > > > prevented. > > > > > This gave me the wrong information about where to go looking for > > > messages > > > > > associated with a problem. > > > > > > > > > > (3) Loggers are meant to be associated with exactly one class; > that's > > > why > > > > > they > > > > > take the class as an argument. Since they're not meant to be used > by > > > > > another > > > > > class, there's no reason for them to have greater visibility. > (There > > > > > certainly > > > > > aren't any examples of code that use "OtherClass.logger....") In > > order > > > to > > > > > reduce the fragility of code, we reduce the visible surface of > > classes > > > > (aka > > > > > "encapsulation"), and that means giving everything the least > > visibility > > > > > that > > > > > it requires -- in this case, "private." > > > > > > > > > > (4) Private loggers reveal when they aren't being used, at least in > > > IDEs. > > > > > When they're private, and they aren't used, we can comment them out > > > > > // private final static Logger logger = ....; > > > > > and avoid allocating them when we don't need them. If we need them, > > > then > > > > we > > > > > can uncomment them. > > > > > > > > > > Does anyone object to making all the loggers in the Drill codebase > > > > private > > > > > from now on? > > > > > > > > > > Chris Westin > > > > > > > > > > > > > > > > > > > > > -- > > > Steven Phillips > > > Software Engineer > > > > > > mapr.com > > > > > >
