drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jason Altekruse" <altekruseja...@gmail.com>
Subject Re: Review Request 29278: Drill-1904 package level docs
Date Tue, 27 Jan 2015 01:40:46 GMT


> On Jan. 16, 2015, 11:23 p.m., Ted Dunning wrote:
> > common/src/main/java/org/apache/drill/common/config/package-info.java, line 21
> > <https://reviews.apache.org/r/29278/diff/3/?file=823063#file823063line21>
> >
> >     Is nesting the salient character for this package?  Is there anything else that
would be surprising?

Added some more information about how config files are persisted and a referral to the class
responsible for finding the files on the classpath/resolving configuration precedence.


> On Jan. 16, 2015, 11:23 p.m., Ted Dunning wrote:
> > common/src/main/java/org/apache/drill/common/exceptions/package-info.java, line
19
> > <https://reviews.apache.org/r/29278/diff/3/?file=823064#file823064line19>
> >
> >     Probably not even needed.  The name and the content speak for themselves.
> >     
> >     They may be something that is not said by these contents, but the mere fact
that these are exceptions is, so to say, unexceptional.

Fair enough, for consistency I just documented it.


> On Jan. 16, 2015, 11:23 p.m., Ted Dunning wrote:
> > common/src/main/java/org/apache/drill/common/expression/package-info.java, line
19
> > <https://reviews.apache.org/r/29278/diff/3/?file=823067#file823067line19>
> >
> >     Too thin to say anything here.  What is a logical expression?  An expression
with nothing but booleans?  Or an expression in the logical plan.
> >     
> >     Perhaps clarification by amplification is in order.

added additional clarification


> On Jan. 16, 2015, 11:23 p.m., Ted Dunning wrote:
> > common/src/main/java/org/apache/drill/common/expression/visitors/package-info.java,
line 19
> > <https://reviews.apache.org/r/29278/diff/3/?file=823069#file823069line19>
> >
> >     Used in what context?  Who calls these? When are the inserted?

Added some information about where the visitors are used.


> On Jan. 16, 2015, 11:23 p.m., Ted Dunning wrote:
> > common/src/main/java/org/apache/drill/common/logical/FormatPluginConfig.java, line
27
> > <https://reviews.apache.org/r/29278/diff/3/?file=823071#file823071line27>
> >
> >     Suggest "Formats are file formats that have nothing to do with how the file
is stored, such as CSV or protobuf.

Updated comments to clarify the relationship between format plugins and storage plugins.


> On Jan. 16, 2015, 11:23 p.m., Ted Dunning wrote:
> > common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java,
line 27
> > <https://reviews.apache.org/r/29278/diff/3/?file=823072#file823072line27>
> >
> >     A tiny bit more would be nice.  "To use this base class, sub-class it and define
....  Watch out fo r ..."

This actually isn't being used as the base class for anything. Opened a JIRA to review the
current structure and removed the docs for now.

https://issues.apache.org/jira/browse/DRILL-2076


> On Jan. 16, 2015, 11:23 p.m., Ted Dunning wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/coord/package-info.java, line
19
> > <https://reviews.apache.org/r/29278/diff/3/?file=823086#file823086line19>
> >
> >     Need a bit more here.  Could have a pointer to what sort of coordination is
done and a description of why.
> >     
> >     Is Curator used?

Added curator to the comment. I don't really think we maintain much beside cluster membership
using zookeeper. I think this is a reasonable descritpion for now.


> On Jan. 16, 2015, 11:23 p.m., Ted Dunning wrote:
> > exec/java-exec/src/main/java/org/apache/drill/exec/package-info.java, line 22
> > <https://reviews.apache.org/r/29278/diff/3/?file=823091#file823091line22>
> >
> >     This is a top-level description that should not quote so much as redirect the
reader.  A link to the architecture home page is sufficient for that.  Links to the key pieces
of code to read next would be very helpful as well.

I have fixed the comment to include a link.

I started writing a more complete intro for new developers, but it was getting a bit long
without covering the breadth I wanted. I have opened a JIRA for better starting documentation
for new developers here as well as on the Wiki. I would like to get this patch in sooner to
encourge others to write docs with a consistent style and format, but I will be trying to
fit in an improvement to this documentation soon.

https://issues.apache.org/jira/browse/DRILL-2077


- Jason


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29278/#review68501
-----------------------------------------------------------


On Jan. 27, 2015, 1:36 a.m., Jason Altekruse wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29278/
> -----------------------------------------------------------
> 
> (Updated Jan. 27, 2015, 1:36 a.m.)
> 
> 
> Review request for drill, Daniel Barclay and Jacques Nadeau.
> 
> 
> Bugs: Drill-1904
>     https://issues.apache.org/jira/browse/Drill-1904
> 
> 
> Repository: drill-git
> 
> 
> Description
> -------
> 
> Add package level documentation to the sources of Drill
> 
> 
> Diffs
> -----
> 
>   common/src/main/java/org/apache/drill/common/config/package-info.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/exceptions/package-info.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/expression/aggregate/BasicAggregates.java
5f18aaf 
>   common/src/main/java/org/apache/drill/common/expression/fn/package-info.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/expression/package-info.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/expression/types/package-info.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/expression/visitors/package-info.java
PRE-CREATION 
>   common/src/main/java/org/apache/drill/common/graph/package-info.java PRE-CREATION 
>   common/src/main/java/org/apache/drill/common/logical/FormatPluginConfig.java 936e4f2

>   common/src/main/java/org/apache/drill/common/logical/FormatPluginConfigBase.java 0704085

>   common/src/main/java/org/apache/drill/common/logical/data/package-info.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/logical/data/visitors/package-info.java
PRE-CREATION 
>   common/src/main/java/org/apache/drill/common/logical/package-info.java PRE-CREATION

>   common/src/main/java/org/apache/drill/common/package-info.java PRE-CREATION 
>   common/src/main/java/org/apache/drill/common/types/package-info.java PRE-CREATION 
>   common/src/main/java/org/apache/drill/common/util/package-info.java PRE-CREATION 
>   contrib/storage-mongo/src/main/java/org/apache/drill/exec/store/mongo/package-info.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/cache/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/client/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/compile/bytecode/package-info.java
PRE-CREATION 
>   exec/java-exec/src/main/java/org/apache/drill/exec/compile/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/compile/sig/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/coord/local/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/coord/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/coord/zk/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/disk/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/dotdrill/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/exception/package-info.java PRE-CREATION

>   exec/java-exec/src/main/java/org/apache/drill/exec/package-info.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/29278/diff/
> 
> 
> Testing
> -------
> 
> Just comments, no code changes
> 
> 
> Thanks,
> 
> Jason Altekruse
> 
>


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