You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by Daniel Barclay <db...@maprtech.com> on 2015/01/20 01:55:32 UTC
Review Request 30060: Documented NamedThreadFactory; cleaned up a bit.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30060/
-----------------------------------------------------------
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
Re: Review Request 30060: Documented NamedThreadFactory;
cleaned up a bit.
Posted by Daniel Barclay <db...@maprtech.com>.
> 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
>
>
Re: Review Request 30060: Documented NamedThreadFactory;
cleaned up a bit.
Posted by Jacques Nadeau <ja...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/30060/#review68684
-----------------------------------------------------------
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/NamedThreadFactory.java
<https://reviews.apache.org/r/30060/#comment113034>
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.
exec/java-exec/src/main/java/org/apache/drill/exec/rpc/NamedThreadFactory.java
<https://reviews.apache.org/r/30060/#comment113033>
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.
- Jacques Nadeau
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
>
>