Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23848 )
Change subject: IMPALA-14942: [tools] Impala devcontainer ...................................................................... Patch Set 52: (8 comments) http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/Dockerfile File .devcontainer-build/Dockerfile: http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/Dockerfile@158 PS52, Line 158: && find "${IMPALA_LOGS_DIR}" -type f | xargs rm \ nit: Using the delete option of find is more robust find "${IMPALA_LOGS_DIR}" -type f -delete http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/Dockerfile@184 PS52, Line 184: CMD ["${IMPALA_HOME}/.devcontainer/entrypoint.sh"] entrypoint.sh doesn't keep the container alive. Do we really need this? postStartCommand in devcontainer/devcontainer.json also runs this. http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/README.md File .devcontainer-build/README.md: http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/README.md@34 PS52, Line 34: neccesary nit: "necessary" http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/build-devcontainer.sh File .devcontainer-build/build-devcontainer.sh: http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/build-devcontainer.sh@99 PS52, Line 99: GIT_REMOTE_NAME="$(git config --get "branch.${GIT_BRANCH}.remote")" This fails on a local branch that has no tracking remote configured. This is only used to get GIT_REPO which seems unused? I see we hard coded https://github.com/apache/impala.git in .devcontainer-build/Dockerfile https://gerrit.cloudera.org/c/23848/52/.devcontainer-build/Dockerfile#164 http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/zshrc File .devcontainer-build/zshrc: http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer-build/zshrc@41 PS52, Line 41: uname -p Needs `uname -m` like we did in .devcontainer-build/build-devcontainer.sh http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer/devcontainer.json File .devcontainer/devcontainer.json: http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer/devcontainer.json@18 PS44, Line 18: */ > VSCode allows for comments in JSON files. JSON files with comments do not I'm OK with either way. But it'd be better to be consistent with .devcontainer-build/devcontainer.json which doesn't have this header. http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer/devcontainer.json@23 PS44, Line 23: type=volume > type=volume is used because the pre-built devcontainer image contains a cop Ack http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer/entrypoint.sh File .devcontainer/entrypoint.sh: http://gerrit.cloudera.org:8080/#/c/23848/52/.devcontainer/entrypoint.sh@25 PS52, Line 25: ] nit: redundant "]" ? -- To view, visit http://gerrit.cloudera.org:8080/23848 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I6aa834494003619fcdfbd98f3bfc7c74d3e1554c Gerrit-Change-Number: 23848 Gerrit-PatchSet: 52 Gerrit-Owner: Jason Fehr <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Gowthami Bisati <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 16 Sep 2026 05:34:58 +0000 Gerrit-HasComments: Yes
