You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/03/30 01:24:18 UTC
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Dan Burkert has uploaded this change for review. ( http://gerrit.cloudera.org:8080/9861
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
KUDU-2191 (6/n): Fixups to the C++ HMS client
These are changes necessary for subsequent patches in the series, but
they are easier to review in isolation.
Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
---
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
5 files changed, 80 insertions(+), 12 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/9861/1
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9861 )
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
KUDU-2191 (6/n): Fixups to the C++ HMS client
These are changes necessary for subsequent patches in the series, but
they are easier to review in isolation.
Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Reviewed-on: http://gerrit.cloudera.org:8080/9861
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
5 files changed, 79 insertions(+), 11 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9861 )
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client-test.cc@20
PS1, Line 20: #include <algorithm>
So IWYU wants you to remove all of these new includes. Are all of them necessary to compile this particular patch (as opposed to others in the series)? If so, could you annotate each one with the appropriate IWYU pragma?
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client.h@22
PS1, Line 22: #include <vector>
My guess is that this is already included through ThriftHiveMetastore.h, which is why IWYU wants you to remove it.
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client.h@33
PS1, Line 33: class NotificationEvent;
: class Partition;
What happens if you drop these? Perhaps they're declared/defined in the ThriftHiveMetastore.h chain?
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 16:59:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9861 )
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
Patch Set 2:
> Patch Set 2: Code-Review+2
>
> But would still like to understand what's going on with IWYU, otherwise IWYU failures are pretty much a guarantee on all future edits to these files.
>
> Same applies for the other patches in the stack, I guess.
We discussed this offline and decided to push pragmas to fix the failures, and circle back when Alexey, the IWYU expert, is available. I have a feeling the imminent IWYU version bump will also be a factor, so it's worth waiting for that.
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 20:52:05 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9861 )
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
Patch Set 2: Code-Review+2
But would still like to understand what's going on with IWYU, otherwise IWYU failures are pretty much a guarantee on all future edits to these files.
Same applies for the other patches in the stack, I guess.
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 19:36:32 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9861 )
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client-test.cc@20
PS1, Line 20: #include <algorithm>
> So IWYU wants you to remove all of these new includes. Are all of them nece
They aren't necessary to compile the patch for the most part, but if I exclude them then IWYU on va1022 complains. For the most part I think they all make sense. The only one I can't quite see is type_traits, but the rules around that are complex.
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client.h@22
PS1, Line 22: #include <vector>
> My guess is that this is already included through ThriftHiveMetastore.h, wh
It doesn't appear to be, nor do I think would that matter. vector is used in multiple places in this header so I think it's correct to include it.
http://gerrit.cloudera.org:8080/#/c/9861/1/src/kudu/hms/hms_client.h@33
PS1, Line 33: class NotificationEvent;
: class Partition;
> What happens if you drop these? Perhaps they're declared/defined in the Thr
It still seems to compile fine, but iwyu on va1022 then warns that you must include them. I think va1022 is right; these symbols are used below.
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 16:56:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Hao Hao, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9861
to look at the new patch set (#3).
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
KUDU-2191 (6/n): Fixups to the C++ HMS client
These are changes necessary for subsequent patches in the series, but
they are easier to review in isolation.
Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
---
M java/kudu-hive/src/test/java/org/apache/kudu/hive/metastore/TestKuduMetastorePlugin.java
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
5 files changed, 79 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/61/9861/3
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9861 )
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Apr 2018 21:08:08 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9861 )
Change subject: KUDU-2191 (6/n): Fixups to the C++ HMS client
......................................................................
Patch Set 1:
This was split out from https://gerrit.cloudera.org/c/8312/, it contains the changes to hms_client.h/cc. I don't think it contains new that the other review didn't.
--
To view, visit http://gerrit.cloudera.org:8080/9861
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86a8d984e67fe6b4c004725828b4296476c2389e
Gerrit-Change-Number: 9861
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 30 Mar 2018 02:43:21 +0000
Gerrit-HasComments: No