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/04/02 16:56:03 UTC

[kudu-CR] KUDU-2191 (6/n): Fixups to the C++ HMS client

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