Quanlong Huang has posted comments on this change. ( http://gerrit.cloudera.org:8080/23848 )
Change subject: IMPALA-14942: [tools] Impala devcontainer ...................................................................... Patch Set 44: (8 comments) http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer-build/Dockerfile File .devcontainer-build/Dockerfile: http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer-build/Dockerfile@130 PS44, Line 130: && rm -f be/build/debug/util/impala-profile-tool \ After IMPALA-12955, the size of impala-profile-tool is small (13MB in my env). I think we don't need to remove it now. http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer-build/Dockerfile@145 PS44, Line 145: --catalogd_args="-iceberg_allow_datafiles_in_table_location_only=false" \ Any reason we need this flag in loading test data? http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer-build/Dockerfile@167 PS44, Line 167: $(echo -n "HMS${IMPALA_HOME//\//_}_cdp" | tr -s "_" "_") Can we use $METASTORE_DB directly? It's set in bin/impala-config.sh http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer-build/zshrc File .devcontainer-build/zshrc: http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer-build/zshrc@164 PS44, Line 164: alias restart-minicluster='pushd "${IMPALA_HOME}" && ./testdata/bin/run-all.sh; popd' Usually we don't need to launch all the services. E.g. for an Impala-only feature, we just need HDFS+HIVE testdata/bin/run-mini-dfs.sh && testdata/bin/run-hive-server.sh It'd be useful to add an alias for this. http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer/README.md File .devcontainer/README.md: http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer/README.md@45 PS44, Line 45: The Impala devcontainer leverages a docker volume which is not cleaned up when the devcontainer is destroyed. Thus, the command `docker volume prune` must be run whenever a devcontainer is destroyed. Can we mention that local changes in devcontainer will also be removed? For people get used to the default bind mode, this is something they need to be aware. 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: */ Do we need this header which seems to make this JSON file invalid? I thought all JSON files are excluded in RAT https://github.com/apache/impala/blob/218b32e2cbafdcb9013a5a2ddd613788bf524286/bin/rat_exclude_files.txt#L131 If not, we can add such files to bin/rat_exclude_files.txt http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer/devcontainer.json@23 PS44, Line 23: type=volume I like the default bind mode since I have many branches in my local repo which then can show up in the container. Destroying the container also safely leave the changes in my laptop. Is using type=volume mainly for performance reason? http://gerrit.cloudera.org:8080/#/c/23848/44/.devcontainer/devcontainer.json@99 PS44, Line 99: "10001:10001" Can we add 10002 which is the WebUI port of HiveServer2? -- 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: 44 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: Sun, 23 Aug 2026 12:52:13 +0000 Gerrit-HasComments: Yes
