Copilot commented on code in PR #61968:
URL: https://github.com/apache/doris/pull/61968#discussion_r3020538758
##########
thirdparty/vars.sh:
##########
@@ -48,6 +48,7 @@ export TP_JAR_DIR="${TP_INSTALL_DIR}/lib/jar"
# source of all dependencies, default unuse it
# export REPOSITORY_URL=
+DORIS_THIRDPARTY_REPOSITORY_URL="${DORIS_THIRDPARTY_REPOSITORY_URL:-https://doris-regression-hk.oss-cn-hongkong.aliyuncs.com/regression/datalake/thirdparty/juicefs}";
Review Comment:
`DORIS_THIRDPARTY_REPOSITORY_URL` is set to a JuiceFS-specific mirror root,
but the name reads like a general thirdparty repository URL (and it sits next
to the commented `REPOSITORY_URL`). This can confuse users/configuration and
makes future extension harder; consider renaming to something JuiceFS-scoped
(e.g. `DORIS_JUICEFS_REPOSITORY_URL` / `JUICEFS_THIRDPARTY_REPOSITORY_URL`) and
updating the dependent variables/tests accordingly.
```suggestion
JUICEFS_THIRDPARTY_REPOSITORY_URL="${JUICEFS_THIRDPARTY_REPOSITORY_URL:-https://doris-regression-hk.oss-cn-hongkong.aliyuncs.com/regression/datalake/thirdparty/juicefs}";
DORIS_THIRDPARTY_REPOSITORY_URL="${DORIS_THIRDPARTY_REPOSITORY_URL:-${JUICEFS_THIRDPARTY_REPOSITORY_URL}}"
```
##########
docker/thirdparties/juicefs-helpers.sh:
##########
@@ -19,7 +19,20 @@
# Shared JuiceFS helper functions used by build and docker scripts.
JUICEFS_DEFAULT_VERSION="${JUICEFS_DEFAULT_VERSION:-1.3.1}"
+JUICEFS_THIRDPARTY_REPOSITORY_URL="${JUICEFS_THIRDPARTY_REPOSITORY_URL:-}"
+JUICEFS_DEFAULT_THIRDPARTY_REPOSITORY_URL="${JUICEFS_DEFAULT_THIRDPARTY_REPOSITORY_URL:-https://doris-regression-hk.oss-cn-hongkong.aliyuncs.com/regression/datalake/thirdparty/juicefs}";
JUICEFS_HADOOP_MAVEN_REPO="${JUICEFS_HADOOP_MAVEN_REPO:-https://repo1.maven.org/maven2/io/juicefs/juicefs-hadoop}";
+JUICEFS_CLI_RELEASE_REPO="${JUICEFS_CLI_RELEASE_REPO:-https://github.com/juicedata/juicefs/releases/download}";
+
+juicefs_thirdparty_repository_url() {
+ local
repository_url="${JUICEFS_THIRDPARTY_REPOSITORY_URL:-${REPOSITORY_URL:-${JUICEFS_DEFAULT_THIRDPARTY_REPOSITORY_URL}}}"
+ echo "${repository_url%/}"
+}
Review Comment:
The PR description says we should prefer the datalake HK OSS mirror by
default, but this URL-precedence makes `REPOSITORY_URL` override the JuiceFS
mirror whenever it is set. If the intent is to avoid relying on the shared
thirdparty mirror for JuiceFS, consider preferring the JuiceFS mirror over
`REPOSITORY_URL` (or clarify in the PR description that `REPOSITORY_URL` takes
precedence).
##########
thirdparty/download-thirdparty.sh:
##########
@@ -177,22 +193,19 @@ for TP_ARCH in "${TP_ARCHIVES[@]}"; do
fi
NAME="${TP_ARCH}_NAME"
MD5SUM="${TP_ARCH}_MD5SUM"
- if [[ -z "${REPOSITORY_URL}" ]]; then
- URL="${TP_ARCH}_DOWNLOAD"
- if ! download_func "${!NAME}" "${!URL}" "${TP_SOURCE_DIR}"
"${!MD5SUM}"; then
- echo "Failed to download ${!NAME}"
- exit 1
- fi
- else
- URL="${REPOSITORY_URL}/${!NAME}"
- if ! download_func "${!NAME}" "${URL}" "${TP_SOURCE_DIR}"
"${!MD5SUM}"; then
- #try to download from home
- URL="${TP_ARCH}_DOWNLOAD"
- if ! download_func "${!NAME}" "${!URL}" "${TP_SOURCE_DIR}"
"${!MD5SUM}"; then
- echo "Failed to download ${!NAME}"
- exit 1 # download failed again exit.
- fi
- fi
+ URL="${TP_ARCH}_DOWNLOAD"
+ FALLBACK_URL="${TP_ARCH}_FALLBACK_DOWNLOAD"
+ DOWNLOAD_URLS=()
+ if [[ -n "${REPOSITORY_URL}" ]]; then
+ DOWNLOAD_URLS+=("${REPOSITORY_URL%/}/${!NAME}")
+ fi
+ DOWNLOAD_URLS+=("${!URL}")
+ if [[ -n "${!FALLBACK_URL}" ]]; then
+ DOWNLOAD_URLS+=("${!FALLBACK_URL}")
+ fi
Review Comment:
`download-thirdparty.sh` still prepends `${REPOSITORY_URL%/}/${NAME}` ahead
of the per-archive `${TP_ARCH}_DOWNLOAD` URL. For JuiceFS this means the shared
thirdparty mirror will still be tried first whenever `REPOSITORY_URL` is set,
which seems to conflict with the stated goal of preferring the dedicated
JuiceFS mirror. Consider reordering for JuiceFS (or documenting that
`REPOSITORY_URL` intentionally has highest priority).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
