Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10817 )
Change subject: WIP: KUDU-2191: support table-name identifiers with upper case chars ...................................................................... Patch Set 5: (11 comments) http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.h File src/kudu/hms/hms_catalog.h: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.h@131 PS3, Line 131: : // Valid Kudu/HMS table names consist > Nit: I think you can get by with "only ascii alphanumerics, _, and /" Done http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.h@134 PS3, Line 134: alphanumeri > Normalized Done http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.h@140 PS3, Line 140: tils. > Nit: "name" maybe? You've used 'table' for a hive::Table* in PopulateTable Done http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.h@162 PS3, Line 162: // Parses a Kudu table name into a Hive database and table name. > I think StringPiece is a more idiomatic choice for "pointer to part of a > string". It doesn't have a mutable_data() member (which you'll need for > ToLowerCase) but you can add that. It's not trivial to add mutable_data() since StringPiece's data field is a const pointer. It appears that StringPiece is generally used as a const view into a string. > Also, please update the function doc to explain what the lifecycles of > hms_database and hms_table are now. Done > On second thought, why bother with this optimization? Why not just continue > to return std::strings? This doesn't seem performance critical, so the extra > lifecycle considerations don't seem worth it. It's necessary in order to implement NormalizeTableName as mutating the table name in place. http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/hms/hms_catalog.cc@105 PS3, Line 105: '_', and '/' > Should we at least warn users about how '/' is configurable in the HMS? Or Too much detail IMO. http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/integration-tests/master_hms-itest.cc File src/kudu/integration-tests/master_hms-itest.cc: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/integration-tests/master_hms-itest.cc@477 PS3, Line 477: Kudu first. > Surprised you needed this. Below too. { "..", ".." } compiles here, but not in the ASSERT calls below. My experience with relying on the compiler to infer these things has been platform specific and generally poor, so I reflexively reach for the explicit form now. http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.h@766 PS3, Line 766: by-name map, table-id map, and tablet : // map), and loads t > Nit: feel free to make this vaguer so it needn't be updated whenever the ma Done http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc@318 PS3, Line 318: auto* existing = InsertOrReturnExisting(&catalog_manager_->normalized_table_names_map_, > I'm a little nervous about this; I think there may have been a good reason This line exists nearly unaltered since early 2014, so I don't think there's anything special about the overwriting behavior. I think it's just always been an invariant that you can't have two tables with the same name. https://github.com/apache/kudu/commit/b0af34c92a0ea0b9c9764798d514b6c5aadda7d4 http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc@2304 PS3, Line 2304: normalized_table_name == normalized_new_table_name)) { > Nit: normally I wouldn't advocate for a double negative, but I wonder if th Done http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc@4794 PS3, Line 4794: if (hms::HmsCatalog::IsEnabled()) { > I'm going to add a comment with more color in a comment, but ignoring this Done http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/mini-cluster/external_mini_cluster.h File src/kudu/mini-cluster/external_mini_cluster.h: http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/mini-cluster/external_mini_cluster.h@322 PS3, Line 322: // Enable Hive Metastore integration. : // Overrides HMS integration options set by ExternalMiniClusterOptions. : // The cluster must be shut down before calling this method. : void EnableMetastoreIntegration(); : : // Disable Hive Metastore integration. : // Overrides HMS integration option > These should probably doc that the cluster must be restarted for the change Done -- To view, visit http://gerrit.cloudera.org:8080/10817 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I18977d6fe7b2999a36681a728ac0d1e54b7f38cd Gerrit-Change-Number: 10817 Gerrit-PatchSet: 5 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: Wed, 27 Jun 2018 23:57:04 +0000 Gerrit-HasComments: Yes
