Laszlo Gaal has posted comments on this change. ( http://gerrit.cloudera.org:8080/15624 )
Change subject: Use Python from the toolchain for Impala ...................................................................... Patch Set 10: (8 comments) http://gerrit.cloudera.org:8080/#/c/15624/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15624/6//COMMIT_MSG@17 PS6, Line 17: native > Nit: native Done http://gerrit.cloudera.org:8080/#/c/15624/6//COMMIT_MSG@20 PS6, Line 20: Python2 : version from the toolchain > This is larger question, but would we ever consider having both python3 and Python3 is already built in the toolchain in addition to Python2 (see https://github.com/cloudera/native-toolchain/blob/master/buildall.sh#L71), although I'm not sure how much testing it gets, if any. http://gerrit.cloudera.org:8080/#/c/15624/6/bin/set-pythonpath.sh File bin/set-pythonpath.sh: http://gerrit.cloudera.org:8080/#/c/15624/6/bin/set-pythonpath.sh@34 PS6, Line 34: PYTHONPATH=${PYTHONPATH}:${IMPALA_HOME}/infra/python/env/lib > Should this be .../infra/python/env/lib ? Done http://gerrit.cloudera.org:8080/#/c/15624/6/infra/python/bootstrap_virtualenv.py File infra/python/bootstrap_virtualenv.py: http://gerrit.cloudera.org:8080/#/c/15624/6/infra/python/bootstrap_virtualenv.py@50 PS6, Line 50: LOG = logging.getLogger(os.path.splitext(os.path.basename(__file__))[0]) : : DEPS_DIR = os.path.join(os.path.dirname(__file__), "deps") : ENV_DIR = os.path.join(os.path. > I think this was for my debugging only, so we can remove it. Done http://gerrit.cloudera.org:8080/#/c/15624/6/infra/python/bootstrap_virtualenv.py@92 PS6, Line 92: def exec_cmd(args, **kwargs): : '''Executes a command and > Can these lines be removed? I think Tim suggested that we didn't need tmp.p Done http://gerrit.cloudera.org:8080/#/c/15624/6/infra/python/bootstrap_virtualenv.py@209 PS6, Line 209: if not os.path.exists(python_cmd): > This feels like a bit of a hack, but I think we can live with it. Ack http://gerrit.cloudera.org:8080/#/c/15624/6/infra/python/tmp.py File infra/python/tmp.py: PS6: > I think we can remove this, this was just for debugging issues I was having Done http://gerrit.cloudera.org:8080/#/c/15624/6/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/15624/6/tests/common/impala_test_suite.py@156 PS6, Line 156: cls.client = None > I almost feel like this check might better be done via a fixture in conftes Done -- To view, visit http://gerrit.cloudera.org:8080/15624 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ic7b40cef89cfb3b467b61b2d54a94e708642882b Gerrit-Change-Number: 15624 Gerrit-PatchSet: 10 Gerrit-Owner: Laszlo Gaal <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Wed, 08 Apr 2020 18:50:47 +0000 Gerrit-HasComments: Yes
