impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Russell (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] Release note updates for Impala 2.8
Date Fri, 27 Jan 2017 01:00:57 GMT
John Russell has posted comments on this change.

Change subject: Release note updates for Impala 2.8
......................................................................


Patch Set 3:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/5668/1//COMMIT_MSG
Commit Message:

Line 28: Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
> I won't have time to do a thorough review here but this commit message need
Done


http://gerrit.cloudera.org:8080/#/c/5668/1/docs/topics/impala_incompatible_changes.xml
File docs/topics/impala_incompatible_changes.xml:

PS1, Line 91: now
> typo
Done


PS1, Line 111:  There was a period during early experimental versions of Impala + Kudu where

             :             non-primary-key olumns had the <codeph>NOT NULL</codeph
> I don't think this is correct. AFAIK the default behavior is the same and w
I clarified the wording a bit.


PS1, Line 111:  There was a period during early experimental versions of Impala + Kudu where

             :             non-primary-key olumns had the <codeph>NOT NULL</codeph
> The GA behavior is columns are nullable by default, unless they are part of
Done


http://gerrit.cloudera.org:8080/#/c/5668/3/docs/topics/impala_incompatible_changes.xml
File docs/topics/impala_incompatible_changes.xml:

PS3, Line 58: Impala Incompatible Changes Introduced in Impala 2.8.x
> This wording is surprising to me. Why is the first word needed at all?
There was something I saw relating to cross-reference links, but I think that was more applicable
in Cloudera-specific docs. I removed the leading "Impala" globally.


Line 65:             They were output in uppercase by mistake, but only for a single Impala
release (Impala 2.7).
> It makes changes easier to review when the lines are 90 characters or fewer
Good point. I'm going to wait and do that globally as a separate gerrit review. I suggest
for entirely new text like this, go into gerrit settings and set 'left side' to 'hide'.


PS3, Line 111:  
> space at end of line
Done


Line 112:             non-primary-key olumns had the <codeph>NOT NULL</codeph>
attribute by default.
> "olumns"
Done


Line 1523:   <concept id="incompatible_changes_07" audience="hidden">
> Why hide this?
The beta releases are so intertwined with CDH and CM release numbers, I want to make a blanket
rule to hide or remove such stale historical info. It is more of a Cloudera convention to
let release notes accrete forever. Contrast with Apache Kudu - https://kudu.apache.org/docs/release_notes.html
- where only the changes for the current release are listed in the docs for that release.


http://gerrit.cloudera.org:8080/#/c/5668/1/docs/topics/impala_new_features.xml
File docs/topics/impala_new_features.xml:

PS1, Line 66: The <codeph>COMPUTE STATS</codeph> statement can
            :                 take advantage of multithreading.
> Not sure if you want to be more specific here as the default for Parquet ta
Done


PS1, Line 90: 
> move left?
You mean indentation-wise? For the XML tagging in the docs, I use Berkeley-style indenting,
e.g.

<p>
  some text
</p>

is the equivalent of

{
  some C code
}


PS1, Line 91: >
            :             <li>
            :               <p rev="IMPALA-4397">
> SORTBY adds ordering for non-partition key columns to better support the ef
Done


PS1, Line 103: that use dynamic 
> move to p?
Good catch. I use <p> even where optional inside list items, and prefer to apply attributes
on the inner <p> tags, because there's the possibility of reusing those paragraphs verbatim
in the detailed writeup of each feature.


PS1, Line 238: STAMP</codeph>, <codeph>DECIMAL<
> needed?
Clunky wording, I'll reword just to emphasize not to worry about inserting data in small batches
like we warn people about for other kinds of tables.


PS1, Line 253: of the Kudu data makes it more efficient than with HDFS
> dimitris should review
I'll mention that Sentry can be bypassed and then have Dimitris review the subsequent patch
set.


PS1, Line 290: 
             :             <li>
Is this purely a CM feature? If so I won't mention it in the upstream docs.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell <jrussell@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: Ambreen Kazi <ambreen.kazi@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Dimitris Tsirogiannis <dtsirogiannis@cloudera.com>
Gerrit-Reviewer: Greg Rahn <grahn@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: John Russell <jrussell@cloudera.com>
Gerrit-Reviewer: Laurel Hale <laurel@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message