Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17094 )
Change subject: IMPALA-9218: Add support for locally compiled Hive ...................................................................... Patch Set 2: (3 comments) This makes sense to me. Can you add the new HIVE overrides to our README-build.md? https://github.com/apache/impala/blob/master/README-build.md The other thing we may want to mention is that the HIVE_VERSION_OVERRIDE does need to be 3.* at the moment due to how we use IMPALA_HIVE_MAJOR_VERSION (though supporting 4.* will happen in the future). http://gerrit.cloudera.org:8080/#/c/17094/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17094/2//COMMIT_MSG@23 PS2, Line 23: export HIVE_HOME_OVERRIDE=~/hive/packaging/target/apache-hive-3.1.3000.7.1.1.0-SNAPSHOT-bin/apache-hive-3.1.3000.7.1.1.0-SNAPSHOT-bin > Not sure what the best choice here is - If the long lines were for the description of what the patch is doing, that would bother me, but this is for a code example. Being a bit long here doesn't bother me much. That said, I don't think very many people would copy-paste this, so if you want to break up the lines, you can do line breaks wherever is easiest. Another option is to use a shorter version number. In other words, 3.1.0-SNAPSHOT rather than 3.1.0.3000.7.1.1.0-SNAPSHOT. http://gerrit.cloudera.org:8080/#/c/17094/2/bin/bootstrap_toolchain.py File bin/bootstrap_toolchain.py: http://gerrit.cloudera.org:8080/#/c/17094/2/bin/bootstrap_toolchain.py@469 PS2, Line 469: use_override_hive = "HIVE_VERSION_OVERRIDE" in os.environ \ : and os.environ["HIVE_VERSION_OVERRIDE"] != "" Nit: for this, I think I would break the line right after the first equals, like: use_override_hive = \ "HIVE_VERSION_OVERRIDE" in os.environ and os.environ["HIVE_VERSION_OVERRIDE"] != "" I think that still fits in 90 characters. http://gerrit.cloudera.org:8080/#/c/17094/2/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/17094/2/bin/impala-config.sh@254 PS2, Line 254: export IMPALA_HIVE_STORAGE_API_VERSION=${HIVE_STORAGE_API_VERSION_OVERRIDE:-"2.3.0.$IMPALA_HIVE_VERSION"} > I've tried to use good taste in the line length here. I know there is a rec The indentation here doesn't bother me. Shell scripts make this hard. -- To view, visit http://gerrit.cloudera.org:8080/17094 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I21892c153c445e3a5d93f2bc8f5e0b799929dd34 Gerrit-Change-Number: 17094 Gerrit-PatchSet: 2 Gerrit-Owner: Anonymous Coward <[email protected]> Gerrit-Reviewer: Anonymous Coward <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Comment-Date: Mon, 22 Feb 2021 19:58:53 +0000 Gerrit-HasComments: Yes
