Michael Smith has posted comments on this change. ( http://gerrit.cloudera.org:8080/20682 )
Change subject: IMPALA-9086: Show Hive configurations in /hadoop-varz page ...................................................................... Patch Set 6: (4 comments) http://gerrit.cloudera.org:8080/#/c/20682/6/be/src/catalog/catalog-server.cc File be/src/catalog/catalog-server.cc: http://gerrit.cloudera.org:8080/#/c/20682/6/be/src/catalog/catalog-server.cc@195 PS6, Line 195: const string HADOOP_VARZ_TEMPLATE = "hadoop-varz.tmpl"; It'd be nice to have some of the tools like search and pagination that you get on /varz (flags.tmpl). But it looks like this is an existing template and you're just expanding it, so that could be left as a separate improvement. http://gerrit.cloudera.org:8080/#/c/20682/6/be/src/catalog/catalog-server.cc@1092 PS6, Line 1092: if (!status.ok()) return; We should log and return an error if this fails. Returning an error looks like Value error(status.GetDetail().c_str(), document->GetAllocator()); document->AddMember("error", error, document->GetAllocator()); return; http://gerrit.cloudera.org:8080/#/c/20682/6/be/src/catalog/catalog-server.cc@1096 PS6, Line 1096: for (const ConfigMap::value_type& config: response.configs) { I think 'const auto&' would be acceptable here. value_type is no more illuminating. http://gerrit.cloudera.org:8080/#/c/20682/6/fe/src/main/java/org/apache/impala/service/JniFrontend.java File fe/src/main/java/org/apache/impala/service/JniFrontend.java: http://gerrit.cloudera.org:8080/#/c/20682/6/fe/src/main/java/org/apache/impala/service/JniFrontend.java@a628 PS6, Line 628: Why remove this comment? HIVE_CONF isn't replacing all uses of CONF. Although I think the function name probably needs to be updated to getHadoopConfig (changed in dc5a33f18ca05c014cde443ef90b35643e5ec50f). -- To view, visit http://gerrit.cloudera.org:8080/20682 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5af0eb68e71afeed64660d4d40584208ea503217 Gerrit-Change-Number: 20682 Gerrit-PatchSet: 6 Gerrit-Owner: Saurabh Katiyal <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Reviewer: Saurabh Katiyal <[email protected]> Gerrit-Comment-Date: Wed, 22 Nov 2023 23:50:55 +0000 Gerrit-HasComments: Yes
