You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org> on 2017/08/29 17:20:14 UTC

[Impala-ASF-CR] Bump Kudu version to 1c70e5d 

Thomas Tauber-Marshall has uploaded a new change for review.

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

Change subject: Bump Kudu version to 1c70e5d
......................................................................

Bump Kudu version to 1c70e5d

This required adding a few extra includes due to a recent commit in
Kudu that rearranged includes to reduce compile times.

Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
---
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-util.cc
M be/src/exprs/kudu-partition-expr.h
M bin/impala-config.sh
4 files changed, 5 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/7872/1
-- 
To view, visit http://gerrit.cloudera.org:8080/7872
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] Include-what-you-use for Kudu client

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#2).

Change subject: Include-what-you-use for Kudu client
......................................................................

Include-what-you-use for Kudu client

A recent commit in Kudu removed some unnecessary includes from Kudu
client headers to reduce compile times. We were implicitly relying on
those includes, so before we upgrade to a newer version of the client
we need to make the includes explicit on our side to avoid breaking
compilation.

Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
---
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-util.cc
M be/src/exprs/kudu-partition-expr.h
3 files changed, 3 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/7872/2
-- 
To view, visit http://gerrit.cloudera.org:8080/7872
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-5863: Include-what-you-use for Kudu client

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-5863: Include-what-you-use for Kudu client
......................................................................


Patch Set 2:

(1 comment)

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

Line 7: Include-what-you-use for Kudu client
> since this may need to be tracked for backports, please file a JIRA for thi
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Include-what-you-use for Kudu client

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has posted comments on this change.

Change subject: Include-what-you-use for Kudu client
......................................................................


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7872/1/be/src/exprs/kudu-partition-expr.h
File be/src/exprs/kudu-partition-expr.h:

PS1, Line 21: #include <kudu/client/client.h>
            : #include <kudu/common/partial_row.h>
> can you try to just forward declare instead of including here (then includi
That gives an error of the form:
'unique_ptr.h:74:22: error: invalid application of ‘sizeof’ to incomplete type ‘kudu::KuduPartialRow’'


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5863: Include-what-you-use for Kudu client

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

Change subject: IMPALA-5863: Include-what-you-use for Kudu client
......................................................................


IMPALA-5863: Include-what-you-use for Kudu client

A recent commit in Kudu removed some unnecessary includes from Kudu
client headers to reduce compile times. We were implicitly relying on
those includes, so before we upgrade to a newer version of the client
we need to make the includes explicit on our side to avoid breaking
compilation.

Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Reviewed-on: http://gerrit.cloudera.org:8080/7872
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-util.cc
M be/src/exprs/kudu-partition-expr.h
3 files changed, 3 insertions(+), 0 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Matthew Jacobs: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>

[Impala-ASF-CR] IMPALA-5863: Include-what-you-use for Kudu client

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

Change subject: IMPALA-5863: Include-what-you-use for Kudu client
......................................................................


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5863: Include-what-you-use for Kudu client

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

Change subject: IMPALA-5863: Include-what-you-use for Kudu client
......................................................................


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1174/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5863: Include-what-you-use for Kudu client

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

Change subject: IMPALA-5863: Include-what-you-use for Kudu client
......................................................................


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] Include-what-you-use for Kudu client

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

Change subject: Include-what-you-use for Kudu client
......................................................................


Patch Set 2:

(1 comment)

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

Line 7: Include-what-you-use for Kudu client
since this may need to be tracked for backports, please file a JIRA for this even though it's trivial


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] Bump Kudu version to 1c70e5d

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

Change subject: Bump Kudu version to 1c70e5d
......................................................................


Patch Set 1:

(1 comment)

does the code not compile without the header changes? It's best to separate code changes from toolchain version bumps when possible because it makes porting to other branches harder. It's OK if it's not possible.

http://gerrit.cloudera.org:8080/#/c/7872/1/be/src/exprs/kudu-partition-expr.h
File be/src/exprs/kudu-partition-expr.h:

PS1, Line 21: #include <kudu/client/client.h>
            : #include <kudu/common/partial_row.h>
can you try to just forward declare instead of including here (then including in the .cc)? for both kudu::client::KuduPartitioner and kudu::KuduPartialRow? I think that a unique_ptr doesn't need the full class definition.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5863: Include-what-you-use for Kudu client

Posted by "Thomas Tauber-Marshall (Code Review)" <ge...@cloudera.org>.
Thomas Tauber-Marshall has uploaded a new patch set (#3).

Change subject: IMPALA-5863: Include-what-you-use for Kudu client
......................................................................

IMPALA-5863: Include-what-you-use for Kudu client

A recent commit in Kudu removed some unnecessary includes from Kudu
client headers to reduce compile times. We were implicitly relying on
those includes, so before we upgrade to a newer version of the client
we need to make the includes explicit on our side to avoid breaking
compilation.

Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
---
M be/src/exec/kudu-table-sink.cc
M be/src/exec/kudu-util.cc
M be/src/exprs/kudu-partition-expr.h
3 files changed, 3 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/72/7872/3
-- 
To view, visit http://gerrit.cloudera.org:8080/7872
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibd041b783a9b7764f4b251291e0be5ed000485ce
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall <tm...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tm...@cloudera.com>