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 4: (3 comments) 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@328 PS3, Line 328: Substitute("$0 or $1 [id=$2]", (*existing)->ToString(), l.data().name(), table_id)); > Wouldn't it be helpful to show the IDs of both tables? It's a little misleading, but this does in fact show both IDs. TableInfo::ToString does the equivalent of Substitute("$0 [$1]", table_name, table_id). I'd have used that for the 'table' local variable as well, however it doesn't get its table name field set until the Commit call a few lines down, so the table name shows up as empty. http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc@1513 PS3, Line 1513: ); > If you do that, update the log statement on L1525 and possibly elsewhere he Should I just use the normalized name in this error message / elsewhere? I can kind of see it both ways. I want to avoid overly verbose error messages attempting to explain what normalization is, though. I think that will cause more confusion than just picking unormalized or normalized. Normalization is an implementation detail, the public facing explanation should be case preserving, but insensitive on lookup. http://gerrit.cloudera.org:8080/#/c/10817/3/src/kudu/master/catalog_manager.cc@4794 PS3, Line 4794: string normalized_table_name = table_name; > Why ignore_result()? AFAICT every place calling CatalogManager::NormalizeTa I'm going to add a comment with more color in a comment, but ignoring this status is critical to supporting legacy tables after enabling the HMS integration in an existing cluster. -- 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: 4 Gerrit-Owner: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Hao Hao <hao....@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Wed, 27 Jun 2018 19:34:51 +0000 Gerrit-HasComments: Yes
