Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/14018 )
Change subject: Prepare for upgrading to Hive 3 ...................................................................... Patch Set 1: (8 comments) http://gerrit.cloudera.org:8080/#/c/14018/1//COMMIT_MSG Commit Message: PS1: Just curious: why are you wrapping your commit messages so aggressively? The longest line here is 52 characters; I think 70ish (I use 76) is more typical. http://gerrit.cloudera.org:8080/#/c/14018/1//COMMIT_MSG@13 PS1, Line 13: initialized initialize http://gerrit.cloudera.org:8080/#/c/14018/1//COMMIT_MSG@17 PS1, Line 17: MiniHMS Nit: MiniHms (or change MiniHms above to MiniHMS). http://gerrit.cloudera.org:8080/#/c/14018/1/src/kudu/hms/hms_catalog.cc File src/kudu/hms/hms_catalog.cc: http://gerrit.cloudera.org:8080/#/c/14018/1/src/kudu/hms/hms_catalog.cc@233 PS1, Line 233: // NOTE: LIKE filters are used instead of = filters due to HIVE-21614 Does this have a tangible impact on performance (on a non-Derby HMS)? Any way to measure? http://gerrit.cloudera.org:8080/#/c/14018/1/src/kudu/hms/mini_hms.cc File src/kudu/hms/mini_hms.cc: http://gerrit.cloudera.org:8080/#/c/14018/1/src/kudu/hms/mini_hms.cc@158 PS1, Line 158: : // Set HADOOP_OS_TYPE=Linux due to HADOOP-8719. : // Remove after HADOOP-15966 is available (Hadoop 3.1.3+) : env_vars.insert({ "HADOOP_OS_TYPE", "Linux" }); Can this be combined into the env_vars declaration above? http://gerrit.cloudera.org:8080/#/c/14018/1/src/kudu/hms/mini_hms.cc@310 PS1, Line 310: Please rationalize these new parameters above. http://gerrit.cloudera.org:8080/#/c/14018/1/src/kudu/hms/mini_hms.cc@320 PS1, Line 320: : <property> : <name>hive.async.log.enabled</name> : <value>false</value> : </property> I experimented with this but didn't find that it made any difference w.r.t. logging. Is it actually important? Or can it be removed? http://gerrit.cloudera.org:8080/#/c/14018/1/src/kudu/util/subprocess.cc File src/kudu/util/subprocess.cc: http://gerrit.cloudera.org:8080/#/c/14018/1/src/kudu/util/subprocess.cc@726 PS1, Line 726: p.SetEnvVars(env_vars); Could you pass env_vars by value and std::move() it here? -- To view, visit http://gerrit.cloudera.org:8080/14018 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: If43ae2330b3d99374c68bae313a3f8bc070f9c69 Gerrit-Change-Number: 14018 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Tue, 06 Aug 2019 17:51:32 +0000 Gerrit-HasComments: Yes
