drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jacques Nadeau <jacq...@apache.org>
Subject Re: Loggers should be private
Date Tue, 03 Mar 2015 21:57:03 GMT
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 <cwestin@maprtech.com> 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 <hgunes@maprtech.com> 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 <sphillips@maprtech.com>
> > 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 <adi@apache.org> wrote:
> > >
> > > > Sounds good to me.
> > > >
> > > > On Mon, Mar 2, 2015 at 11:28 AM, Chris Westin <
> chriswestin42@gmail.com
> > >
> > > > 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
> > >
> >
>

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