impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Jim Apple (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3398: Rework Impala documentation to be non-Cloudera-specific
Date Tue, 29 Nov 2016 23:18:00 GMT
Jim Apple has posted comments on this change.

Change subject: IMPALA-3398: Rework Impala documentation to be non-Cloudera-specific
......................................................................


Patch Set 2:

(13 comments)

> (12 comments)
 > 
 > Because I'm advocating re-doing the automated portions of this
 > change at a later time, I don't want to promote this patch set
 > as-is. Is it practical to say "abandon" for the code review but
 > still come back and pick out bits and pieces (e.g. the changes in
 > impala_jdbc.xml) to become part of a later code review?

Technically, yes - "Abandon"ed patches remain just as visible and can be resurrected with
one click.

However, the usual way to do things is to just address the comments and upload a new "Patch
Set", as I have done.

http://gerrit.cloudera.org:8080/#/c/5239/2//COMMIT_MSG
Commit Message:

Line 7: IMPALA-3398: Rework Impala documentation to be non-Cloduera-specific
> "Cloudera"
Done


PS2, Line 16:     sed -i 's/ rev=""//g' $FILENAME
> Let's leave this step out, for a couple of reasons:
Done


http://gerrit.cloudera.org:8080/#/c/5239/2/docs/impala_html.ditaval
File docs/impala_html.ditaval:

PS2, Line 6: <prop att="audience" val="Hidden" action="exclude"/>
> That's fine. We will need to make the same addition in the Cloudera doc bui
SGTM.

Keep in mind that this gerrit instance, though hosted at cloudera.org, is for Apache Impala,
so whatever choices Cloudera makes about their git repo is not really in-scope here.


http://gerrit.cloudera.org:8080/#/c/5239/2/docs/topics/impala_alter_table.xml
File docs/topics/impala_alter_table.xml:

PS2, Line 25:       <indexterm audience="Hidden">ALTER TABLE statement</indexterm>
> For the <indexterm> tags, maybe we could change those to audience="HTML" so
I think that's orthogonal to this change. It's usually best if patches address a single item,
along with maybe some other trivial ones, not big ones like "Are index terms included".

As such, I'd suggest it go in another commit and we keep this one focused on excising "Cloudera"/"CDH"/"Cloudera
Manager".


PS2, Line 220:     <p rev="2.3.0 IMPALA-1568">
> The reason I'm so liberal with release numbers and JIRA numbers with this r
SGTM


PS2, Line 406:     <p>
> Here's a case where perhaps there is a corresponding IMPALA- JIRA number we
Done


http://gerrit.cloudera.org:8080/#/c/5239/2/docs/topics/impala_conversion_functions.xml
File docs/topics/impala_conversion_functions.xml:

PS2, Line 30:       Although in <keyword keyref="impala23_full"/>, the <codeph>SHOW
FUNCTIONS</codeph> output for
> Just checking that this was a change introduced by Jim. This is a case wher
Yes, it was.


PS2, Line 443: <dlentry rev="2.3.0" id="casttosmallint" audience="Hidden">
> These castto*() functions might be intended to be permanently for internal 
I think that's an issue for a future patch, since it's a question of docs correctness, not
of ASF/Cloudera separation.


PS2, Line 619: <dlentry rev="2.3.0" id="typeof">
> Hmm, this might have a corresponding IMPALA- JIRA number that should go in 
I'd call this future work.


http://gerrit.cloudera.org:8080/#/c/5239/2/docs/topics/impala_jdbc.xml
File docs/topics/impala_jdbc.xml:

PS2, Line 82:         In Impala 2.0 and later, you must use the Hive 0.13 JDBC driver.  If
you are
            :         already using JDBC applications with an earlier Impala release, you
must update
            :         your JDBC driver, because the Hive 0.12 driver that was formerly the
only choice
            :         is not compatible with Impala
> Urgh. "Must" seems like too strong a statement. I wonder if the right thing
Removed "must"s.

I'd say Apache Impala's docs should talk about open-source JDBC drivers and vendor's docs
should talk about their own JDBC drivers.


PS2, Line 120:           To get the JAR files, install the Hive JDBC driver on each host in
the cluster that will run
> It's worth checking if there's a generic Hadoop or Hive URL we could refer 
Couldn't find one; added TODO


PS2, Line 139:   hive-common-X.XX.X.jar
             :   hive-jdbc-X.XX.X.jar
             :   hive-metastore-X.XX.X.jar
             :   hive-servi
> We should confirm that these filenames are accurate if someone goes the gen
I poked around on Google and it appears so.


PS2, Line 161:               <xref href="https://github.com/onefoursix/Cloudera-Impala-JDBC-Example"
scope="external" format="html">this
> Ironic that there's a reference to "Cloudera Impala" that perhaps we could 
indeed!


-- 
To view, visit http://gerrit.cloudera.org:8080/5239
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib3f63fb309e0617d7fe014231bb0ab0ad67c8474
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Anonymous Coward #250
Gerrit-Reviewer: Greg Rahn <grahn@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: John Russell <jrussell@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message