> On Jan. 20, 2015, 2:48 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/NamedThreadFactory.java,
> > line 37
> > <https://reviews.apache.org/r/30060/diff/1/?file=825996#file825996line37>
> >
> > We use this fully qualified in many many places. We also new package
> > protected in many places. I'm inclined to stick with consistency with
> > the rest of code rather than changing to private declaration and static
> > import.
> We also new package protected in many places.
Loggers are normally (outside of Drill code) private.
Why declare them package-private? Only restricting their visibility to being
package-private means that when we forget to declare a {{logger}} member in a
subclass but still refer to {{logger}} to log something, then we won't get a
compiler error to inform us, and the log output will make it seem like the
logger call came from the superclass when it really came from a subclass.
> We use this fully qualified in many many places.
Why? For everything else, we seem to use importing normally, and avoid
cluttering up code with extra name segments when it's otherwise unnecessary.
Why not be consistent with that with logger declarations (especially since
they are boiler-plate code--common code that shouldn't distract attention
from the surroundings)?
Would you be more comfortable with the import change if it didn't go as far
as using static imports but only used regular imports, resulting in:
{{private static final Logger logger = LoggerFactory.getLogger(Xxx.class);}}
rather than:
{{private static final Logger logger = getLogger(Xxx.class);}}
?
Or is the main objection to the increased inconsistency that would result in
the interim from changing this pattern incrementally rather than to the new
pattern itself? If so, would changing everything at once (no, not as part of
this NamedThreadFactory), resolve that objection?
> On Jan. 20, 2015, 2:48 a.m., Jacques Nadeau wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/rpc/NamedThreadFactory.java,
> > line 75
> > <https://reviews.apache.org/r/30060/diff/1/?file=825996#file825996line75>
> >
> > Remove all of this. I think it is wrong in the case of daemon. I think
> > the max priority stuff is old debugging code. We'll need to run extra set
> > of tests after removal, though.
Roger.
By "extra" tests did you mean another run of the same set of tests, or running
a different sets of tests?
- Daniel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30060/#review68684
-----------------------------------------------------------
On Jan. 20, 2015, 12:55 a.m., Daniel Barclay wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30060/
> -----------------------------------------------------------
>
> (Updated Jan. 20, 2015, 12:55 a.m.)
>
>
> Review request for drill, Jacques Nadeau and Jason Altekruse.
>
>
> Repository: drill-git
>
>
> Description
> -------
>
> Mainly, documented NamedThreadFactory. Also performed minor cleanup.
>
>
> Diffs
> -----
>
>
> exec/java-exec/src/main/java/org/apache/drill/exec/rpc/NamedThreadFactory.java
> 2b49579
>
> Diff: https://reviews.apache.org/r/30060/diff/
>
>
> Testing
> -------
>
> Ran developer and QA tests; got no errors other than errors that appear for
> master branch.
>
>
> Thanks,
>
> Daniel Barclay
>
>
