Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11121 )
Change subject: Add unstable client APIs to fetch HMS integration configuration status ...................................................................... Patch Set 1: (13 comments) http://gerrit.cloudera.org:8080/#/c/11121/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11121/1//COMMIT_MSG@12 PS1, Line 12: The new C++ API is utilized by the HMS tools to : get the HMS connection information without having to use the GetFlags : API, which requires admin privileges. Do you anticipate a paranoid user wanting to restrict access to this information? At the moment it's available to anyone who can authenticate to Kudu, right? http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@298 PS1, Line 298: private HiveMetastoreConfig hiveMetastoreConfig = null; Document. http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@839 PS1, Line 839: public Deferred<HiveMetastoreConfig> getHiveMetastoreConfig() { Is there any way to reuse the guts of exportAuthenticationCredentials here? Perhaps a generic helper whose type is either byte[] or HiveMetastoreConfig, with a passed-in callback which returns something different after the lookup in either case? http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@848 PS1, Line 848: .addCallback(new MasterLookupCB(masterTable, null, 1)) I know this was just copied from exportAuthenticationCredentials, but could you document the importance of null and 1? http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@852 PS1, Line 852: // Connecting to the cluster should have also fetched the : // authn data. Not relevant for this use case. http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1579 PS1, Line 1579: synchronized (AsyncKuduClient.this) { Could you rewrite this block so that the lock is only held around the reset of hiveMetastoreConfig? http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/HiveMetastoreConfig.java File java/kudu-client/src/main/java/org/apache/kudu/client/HiveMetastoreConfig.java: http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/HiveMetastoreConfig.java@27 PS1, Line 27: @InterfaceStability.Evolving Why Evolving and not Unstable? http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java: http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java@93 PS1, Line 93: FakeDNS.getInstance().install(); Is this needed to pacify the HMS? I think it's used in testKerberos for the KDC. http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/client/client-internal.cc@713 PS1, Line 713: hive_config Can't this be null if there's no integration? http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/client/client.h@67 PS1, Line 67: bool GetHiveMetastoreSaslEnabled(const client::KuduClient&); : std::string GetHiveMetastoreUris(const client::KuduClient&); We've been pretty inconsistent on defining private C++ client APIs: 1. KuduClient::GetTablet is a public (in the C++ sense of the word) function but its implementation is not exported. 2. KuduTableAlterer::alter_external_catalogs is a private function with an exported implementation and friendship. 3. A few CLI functions aren't even part of the client API at all; they access the PIMPL'ed data members directly via friendship. These new APIs follow this approach. AFAICT, all uses of approach #2 and #3 were added for HMS integration, so I don't feel too bad asking you to spend a little time making this more consistent. I prefer the approach taken by GetTablet, for a few reasons: - It doesn't require friendship or forward declaration of external functions/classes. - It's a bit harder to abuse (you need to rewrite the shared object instead of a plain text header). - It doesn't require awkwardly passing KuduClient as a function argument. - It also makes it easy to document the function as we've documented the others. How would you feel about changing these new APIs to follow the KUDU_NO_EXPORT approach, and then to add a follow-on patch modifying GetMasterAddresses, SetAlterExternalCatalogs, and KuduTableAlterer::alter_external_catalogs? http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/hms/mini_hms.h File src/kudu/hms/mini_hms.h: http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/hms/mini_hms.h@53 PS1, Line 53: temprorary temporary http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/master/master.proto@665 PS1, Line 665: optional HiveMetastoreConfig hive_metastore_config = 6; I think you can get away with shorter field names (i.e. hms_config instead of hive_metastore_config). Likewise since the contents of HiveMetastoreConfig are namespaced within it, I'd drop the hive_metastore_ prefix altogether. http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/tools/tool.proto File src/kudu/tools/tool.proto: http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/tools/tool.proto@45 PS1, Line 45: // Whether or not the cluster should be integrated with a Hive Metastore. I don't understand this. Isn't this bool less expressive than hms_mode? And the corresponding change to tool_action_test.cc means hms_mode is no longer considered at all. -- To view, visit http://gerrit.cloudera.org:8080/11121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iddb89787ed35c41e85f1d9bf953c4c228dcafcdb Gerrit-Change-Number: 11121 Gerrit-PatchSet: 1 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Sat, 04 Aug 2018 00:00:33 +0000 Gerrit-HasComments: Yes
