You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2017/01/11 02:13:53 UTC

[kudu-CR] Doc review for 1.2 

Hello Jean-Daniel Cryans, Ambreen Kazi,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/5676

to review the following change.

Change subject: Doc review for 1.2
......................................................................

Doc review for 1.2

I made a pass through all of the documentation and made improvements as
necessary for 1.2:

* Specify that the start/stop instructions are deb/rpm specific
* Updated the contribution C++ style guide to stop talking about
  gscoped_ptr
* Removed limitation that Spark is only for Spark 1 (now we support
  Spark 2)
* Fixed some branding in index.adoc
* Some small tweaks and improvements to the glossary of concepts

The larger changes are in the Impala integration. I tried to update the
syntax and examples to match the Impala-Kudu release from 11/22/2016
(the most recent release at the time of this writing and at the expected
release date for Kudu 1.2). Unfortunately, the syntax and capabilities
have changed again between that release and the upcoming Impala 2.8
release. Rather than document the unreleased syntax, I went with the
soon-to-be-removed syntax.

Similarly, I left in all of the instructions about how to install the
Impala-Kudu fork, even though that fork has now been merged back into
the main Impala release train. When Impala 2.8 is available, we will
have to do another pass here and re-publish docs.

Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
---
M docs/administration.adoc
M docs/contributing.adoc
M docs/developing.adoc
M docs/index.adoc
M docs/kudu_impala_integration.adoc
5 files changed, 88 insertions(+), 187 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/5676/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5676
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>

[kudu-CR] Doc review for 1.2

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Doc review for 1.2
......................................................................


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5676/1/docs/kudu_impala_integration.adoc
File docs/kudu_impala_integration.adoc:

PS1, Line 376: DISTRIBUTE BY HASH INTO (id) 16 BUCKETS
> You mean
I'm documenting the 2.7 syntax since 2.8 isn't released yet. But you're right it should say (id)


PS1, Line 413: DISTRIBUTE BY HASH(name) INTO 8 BUCKETS
> Also new syntax:
same (2.7 syntax)


PS1, Line 448: DISTRIBUTE BY RANGE (_id)
> PARTITION BY
I believe that's a misconception


PS1, Line 449: SPLIT ROWS((1439560049342),
             :              (1439566253755),
             :              (1439572458168),
             :              (1439578662581),
             :              (1439584866994),
             :              (1439591071407))
> Now the syntax is different with 'PARTITION x <= VALUES < y'. I'm not sure 
2.7 syntax


PS1, Line 482: '\<', '\>', `>=`, or `IN`
> Does this apply for BETWEEN also, since that acts the same as a couple of c
oh, yea, BETWEEN should get pushed


PS1, Line 724: re-enable when documenting
             : // Impala 2.8.
> Aren't we documenting Impala 2.8 now or is this some intermediate stage?
these docs will release with Kudu 1.2, which will release prior to Impala 2.8 being available. We'll update them again for Impala 2.8 in a couple weeks


PS1, Line 941: 8
> I thought it was still true. You have to alter the TBLPROPERTIES kudu.table
ah, didn't know that.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Doc review for 1.2

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Doc review for 1.2
......................................................................


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5676/1/docs/kudu_impala_integration.adoc
File docs/kudu_impala_integration.adoc:

PS1, Line 448: DISTRIBUTE BY RANGE (_id)
> I believe that's a misconception
double checked, I can do:

[vd0342.halxg.cloudera.com:21000] > create table todd_test (a int primary key) partition by range(a) ( partition values < 100, partition 100 <= values) stored as kudu;


PS1, Line 482: '\<', '\>', `>=`, or `IN`
> oh, yea, BETWEEN should get pushed
Done


PS1, Line 941: 8
> ah, didn't know that.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Doc review for 1.2

Posted by "John Russell (Code Review)" <ge...@cloudera.org>.
John Russell has posted comments on this change.

Change subject: Doc review for 1.2
......................................................................


Patch Set 1:

(7 comments)

Just skimmed quickly on the first pass.

http://gerrit.cloudera.org:8080/#/c/5676/1/docs/kudu_impala_integration.adoc
File docs/kudu_impala_integration.adoc:

PS1, Line 376: DISTRIBUTE BY HASH INTO (id) 16 BUCKETS
> DISTRIBUTE BY HASH (id) PARTITIONS 16
You mean

PARTITION BY HASH (id) PARTITIONS 16

right?


PS1, Line 413: DISTRIBUTE BY HASH(name) INTO 8 BUCKETS
Also new syntax:

PARTITION BY HASH(name) PARTITIONS 8


PS1, Line 448: DISTRIBUTE BY RANGE (_id)
PARTITION BY

In my experiments I thought there wasn't a way to have just range partitioning, that it had to by HASH and then optionally RANGE. Is that a misconception on my part?


PS1, Line 449: SPLIT ROWS((1439560049342),
             :              (1439566253755),
             :              (1439572458168),
             :              (1439578662581),
             :              (1439584866994),
             :              (1439591071407))
Now the syntax is different with 'PARTITION x <= VALUES < y'. I'm not sure how that plays out for the specific partitions in this example.


PS1, Line 482: '\<', '\>', `>=`, or `IN`
Does this apply for BETWEEN also, since that acts the same as a couple of comparisons chained together?


PS1, Line 724: re-enable when documenting
             : // Impala 2.8.
Aren't we documenting Impala 2.8 now or is this some intermediate stage?


PS1, Line 941: 8
I thought it was still true. You have to alter the TBLPROPERTIES kudu.table_name to make Impala rename the underlying Kudu table.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Doc review for 1.2

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: Doc review for 1.2
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5676/1/docs/kudu_impala_integration.adoc
File docs/kudu_impala_integration.adoc:

PS1, Line 449: SPLIT ROWS((1439560049342),
             :              (1439566253755),
             :              (1439572458168),
             :              (1439578662581),
             :              (1439584866994),
             :              (1439591071407))
> 2.7 syntax
oh actually it looks like the latest IMPALA_KUDU release _does_ have the new 'values' syntax. will fix.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] Doc review for 1.2

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: Doc review for 1.2
......................................................................


Doc review for 1.2

I made a pass through all of the documentation and made improvements as
necessary for 1.2:

* Specify that the start/stop instructions are deb/rpm specific
* Updated the contribution C++ style guide to stop talking about
  gscoped_ptr
* Removed limitation that Spark is only for Spark 1 (now we support
  Spark 2)
* Fixed some branding in index.adoc
* Some small tweaks and improvements to the glossary of concepts

The larger changes are in the Impala integration. I tried to update the
syntax and examples to match the Impala-Kudu release from 11/22/2016
(the most recent release at the time of this writing and at the expected
release date for Kudu 1.2). Unfortunately, the syntax and capabilities
have changed again between that release and the upcoming Impala 2.8
release. Rather than document the unreleased syntax, I went with the
soon-to-be-removed syntax.

Similarly, I left in all of the instructions about how to install the
Impala-Kudu fork, even though that fork has now been merged back into
the main Impala release train. When Impala 2.8 is available, we will
have to do another pass here and re-publish docs.

Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Reviewed-on: http://gerrit.cloudera.org:8080/5676
Reviewed-by: Jean-Daniel Cryans <jd...@apache.org>
Tested-by: Kudu Jenkins
---
M docs/administration.adoc
M docs/contributing.adoc
M docs/developing.adoc
M docs/index.adoc
M docs/kudu_impala_integration.adoc
5 files changed, 145 insertions(+), 259 deletions(-)

Approvals:
  Jean-Daniel Cryans: Looks good to me, approved
  Kudu Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Doc review for 1.2

Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.

Change subject: Doc review for 1.2
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Doc review for 1.2

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5676

to look at the new patch set (#3).

Change subject: Doc review for 1.2
......................................................................

Doc review for 1.2

I made a pass through all of the documentation and made improvements as
necessary for 1.2:

* Specify that the start/stop instructions are deb/rpm specific
* Updated the contribution C++ style guide to stop talking about
  gscoped_ptr
* Removed limitation that Spark is only for Spark 1 (now we support
  Spark 2)
* Fixed some branding in index.adoc
* Some small tweaks and improvements to the glossary of concepts

The larger changes are in the Impala integration. I tried to update the
syntax and examples to match the Impala-Kudu release from 11/22/2016
(the most recent release at the time of this writing and at the expected
release date for Kudu 1.2). Unfortunately, the syntax and capabilities
have changed again between that release and the upcoming Impala 2.8
release. Rather than document the unreleased syntax, I went with the
soon-to-be-removed syntax.

Similarly, I left in all of the instructions about how to install the
Impala-Kudu fork, even though that fork has now been merged back into
the main Impala release train. When Impala 2.8 is available, we will
have to do another pass here and re-publish docs.

Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
---
M docs/administration.adoc
M docs/contributing.adoc
M docs/developing.adoc
M docs/index.adoc
M docs/kudu_impala_integration.adoc
5 files changed, 145 insertions(+), 259 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/5676/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5676
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] Doc review for 1.2

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: Doc review for 1.2
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5676/1/docs/kudu_impala_integration.adoc
File docs/kudu_impala_integration.adoc:

PS1, Line 376: DISTRIBUTE BY HASH INTO (id) 16 BUCKETS
DISTRIBUTE BY HASH (id) PARTITIONS 16


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] Doc review for 1.2

Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.

Change subject: Doc review for 1.2
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] Doc review for 1.2

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5676

to look at the new patch set (#2).

Change subject: Doc review for 1.2
......................................................................

Doc review for 1.2

I made a pass through all of the documentation and made improvements as
necessary for 1.2:

* Specify that the start/stop instructions are deb/rpm specific
* Updated the contribution C++ style guide to stop talking about
  gscoped_ptr
* Removed limitation that Spark is only for Spark 1 (now we support
  Spark 2)
* Fixed some branding in index.adoc
* Some small tweaks and improvements to the glossary of concepts

The larger changes are in the Impala integration. I tried to update the
syntax and examples to match the Impala-Kudu release from 11/22/2016
(the most recent release at the time of this writing and at the expected
release date for Kudu 1.2). Unfortunately, the syntax and capabilities
have changed again between that release and the upcoming Impala 2.8
release. Rather than document the unreleased syntax, I went with the
soon-to-be-removed syntax.

Similarly, I left in all of the instructions about how to install the
Impala-Kudu fork, even though that fork has now been merged back into
the main Impala release train. When Impala 2.8 is available, we will
have to do another pass here and re-publish docs.

Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
---
M docs/administration.adoc
M docs/contributing.adoc
M docs/developing.adoc
M docs/index.adoc
M docs/kudu_impala_integration.adoc
5 files changed, 90 insertions(+), 190 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/5676/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5676
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I64ad4925ba4402ef3733a0ab62da91f1049848fb
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Ambreen Kazi <am...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: John Russell <jr...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>