Dan Burkert 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 2: (17 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 inform Right, a major point of this patch is to allow this information to be exposed to normal client users. We already have the concept of 'sensitive' flags which we redact, however we don't take that into account with the GetFlags API, and instead just disallow clients from calling it. I wouldn't consider the HMS connection information to be sensitive, and considering it as such is probably antithetical to proper security (security through obscurity, etc). 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: @GuardedBy("this") > Document. Done http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@834 PS1, Line 834: > most recent connection? added a hyphen to clarify. http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@839 PS1, Line 839: @InterfaceAudience.LimitedPrivate("Impala") > Is there any way to reuse the guts of exportAuthenticationCredentials here? I'm not keen to add an abstraction here, because I think it's papering over a more fundamental issue. The real fix is to unfuck the Java client's connection behavior, at which point these fields would get automatically set (see how straightforward this change was on the C++ side, for comparison). It's also difficult (impossible?) to abstract over reading/writing to fields, and without that the helper would be net more lines. http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@846 PS1, Line 846: > Wrong comment? Done http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@848 PS1, Line 848: // We have no Metastore config -- connect to the master, which will fetch new info. > I know this was just copied from exportAuthenticationCredentials, but could Done http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@852 PS1, Line 852: /* requestedBatchSize */ 1)) : .addCallback(new C > Not relevant for this use case. Done http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@1579 PS1, Line 1579: } > Could you rewrite this block so that the lock is only held around the reset Done 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@26 PS1, Line 26: Impala > Do you foresee this been used by other project? Hopefully not - the goal is that normal client apps should not need to care whether HMS integration is enabled or not. The biggest thing the integration changes is restrictions on table names, but apps should probably always follow the HMS rules. 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.Unstable > Why Evolving and not Unstable? Done 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: try (MiniKuduCluster cluster = new MiniKuduCluster.MiniKuduClusterBuilder() > Is this needed to pacify the HMS? I think it's used in testKerberos for the this was cargo-culted. Removed. http://gerrit.cloudera.org:8080/#/c/11121/1/java/kudu-client/src/test/java/org/apache/kudu/client/TestMiniKuduCluster.java@100 PS1, Line 100: > Also assert the content? There aren't any known values to assert against, as far as I know. 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? No, the accessor will return a reference to a default-populated message. 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: std::string GetMasterAddresses(const client::KuduClient&); : void SetAlterExternalCatalogs(client::KuduTableAlterer*, boo > We've been pretty inconsistent on defining private C++ client APIs: Thanks for pointing this out, KUDU_NO_EXPORT is definitely cleaner. 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 hms_config = 6; > I think you can get away with shorter field names (i.e. hms_config instead Done http://gerrit.cloudera.org:8080/#/c/11121/1/src/kudu/master/master.proto@671 PS1, Line 671: Address > Address(es)? Done 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 to create a Hive Metastore, and/or enable Kudu Hive > +1, not sure why do we need to add this? Done -- 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: 2 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 21 Aug 2018 21:40:38 +0000 Gerrit-HasComments: Yes
