Copilot commented on code in PR #10377:
URL: https://github.com/apache/gravitino/pull/10377#discussion_r2939689821
##########
dev/docker/gravitino/gravitino-dependency.sh:
##########
@@ -95,11 +87,15 @@ find ${gravitino_home}/bundles/aws-bundle/build/libs/ -name
'gravitino-aws-bundl
find ${gravitino_home}/bundles/gcp-bundle/build/libs/ -name
'gravitino-gcp-bundle-*.jar' ! -name '*-empty.jar' -exec cp -v {}
"${fileset_lib_dir}" \;
find ${gravitino_home}/bundles/azure-bundle/build/libs/ -name
'gravitino-azure-bundle-*.jar' ! -name '*-empty.jar' -exec cp -v {}
"${fileset_lib_dir}" \;
-# Copy the Aliyun, AWS, GCP and Azure bundles to the Iceberg REST server libs
-find ${gravitino_home}/bundles/iceberg-gcp-bundle/build/libs/ -name
'gravitino-iceberg-gcp-bundle-*.jar' ! -name '*-empty.jar' -exec cp -v {}
"${gravitino_iceberg_rest_dir}" \;
-find ${gravitino_home}/bundles/iceberg-aws-bundle/build/libs/ -name
'gravitino-iceberg-aws-bundle-*.jar' ! -name '*-empty.jar' -exec cp -v {}
"${gravitino_iceberg_rest_dir}" \;
-find ${gravitino_home}/bundles/iceberg-azure-bundle/build/libs/ -name
'gravitino-iceberg-azure-bundle-*.jar' ! -name '*-empty.jar' -exec cp -v {}
"${gravitino_iceberg_rest_dir}" \;
-find ${gravitino_home}/bundles/iceberg-aliyun-bundle/build/libs/ -name
'gravitino-iceberg-aliyun-bundle-*.jar' ! -name '*-empty.jar' -exec cp -v {}
"${gravitino_iceberg_rest_dir}" \;
+iceberg_bundle_dir="${gravitino_dir}/packages/gravitino/iceberg-bundles"
+
+# Copy the Aliyun, AWS, GCP and Azure bundles to the Iceberg bundles directory
+mkdir -p "${iceberg_bundle_dir}"
+find ${gravitino_home}/bundles/iceberg-gcp-bundle/build/libs/ -name
'gravitino-iceberg-gcp-bundle-*.jar' ! -name '*-empty.jar' -exec cp -v {}
"${iceberg_bundle_dir}" \;
Review Comment:
`iceberg_bundle_dir` is built by repeating the package root
(`${gravitino_dir}/packages/gravitino/...`) even though `gravitino_package_dir`
is available. Prefer `${gravitino_package_dir}/iceberg-bundles` to keep path
construction centralized and less error-prone.
##########
dev/docker/gravitino/start-gravitino.sh:
##########
@@ -26,6 +26,42 @@ cd ${gravitino_dir}
python bin/rewrite_gravitino_server_config.py
+# Create soft links for JDBC drivers
+jdbc_driver_dir="${gravitino_dir}/jdbc-drivers"
+
+if [ -d "${jdbc_driver_dir}" ]; then
+ # Link MySQL driver to catalogs that need it
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-mysql/libs"
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-doris/libs"
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-starrocks/libs"
+ mkdir -p "${gravitino_dir}/catalogs/lakehouse-iceberg/libs"
+ mkdir -p "${gravitino_dir}/iceberg-rest-server/libs"
+ mkdir -p "${gravitino_dir}/libs"
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-mysql/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-doris/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-starrocks/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/lakehouse-iceberg/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/iceberg-rest-server/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/libs/" \;
+
+ # Link PostgreSQL driver to catalogs that need it
Review Comment:
The script creates target directories for MySQL driver links, but not for
the PostgreSQL catalog. If `catalogs/jdbc-postgresql/libs` is missing in some
packaging scenarios, these `ln` commands will fail and abort startup (due to
`set -e`). Consider creating the directory with `mkdir -p` alongside the others.
##########
dev/docker/gravitino/start-gravitino.sh:
##########
@@ -26,6 +26,42 @@ cd ${gravitino_dir}
python bin/rewrite_gravitino_server_config.py
+# Create soft links for JDBC drivers
+jdbc_driver_dir="${gravitino_dir}/jdbc-drivers"
+
+if [ -d "${jdbc_driver_dir}" ]; then
+ # Link MySQL driver to catalogs that need it
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-mysql/libs"
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-doris/libs"
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-starrocks/libs"
+ mkdir -p "${gravitino_dir}/catalogs/lakehouse-iceberg/libs"
+ mkdir -p "${gravitino_dir}/iceberg-rest-server/libs"
+ mkdir -p "${gravitino_dir}/libs"
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-mysql/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-doris/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-starrocks/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/lakehouse-iceberg/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/iceberg-rest-server/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/libs/" \;
+
+ # Link PostgreSQL driver to catalogs that need it
+ find "${jdbc_driver_dir}" -name "postgresql-*.jar" -exec ln -sfv {}
"${gravitino_dir}/catalogs/jdbc-postgresql/libs/" \;
+ find "${jdbc_driver_dir}" -name "postgresql-*.jar" -exec ln -sfv {}
"${gravitino_dir}/catalogs/lakehouse-iceberg/libs/" \;
+ find "${jdbc_driver_dir}" -name "postgresql-*.jar" -exec ln -sfv {}
"${gravitino_dir}/iceberg-rest-server/libs/" \;
+ find "${jdbc_driver_dir}" -name "postgresql-*.jar" -exec ln -sfv {}
"${gravitino_dir}/libs/" \;
+fi
+
+# Create soft links for Iceberg bundle jars
+iceberg_bundle_dir="${gravitino_dir}/iceberg-bundles"
+lakehouse_iceberg_lib_dir="${gravitino_dir}/catalogs/lakehouse-iceberg/libs"
+iceberg_rest_lib_dir="${gravitino_dir}/iceberg-rest-server/libs"
+
+if [ -d "${iceberg_bundle_dir}" ]; then
+ mkdir -p "${lakehouse_iceberg_lib_dir}"
+ mkdir -p "${iceberg_rest_lib_dir}"
+ find ${iceberg_bundle_dir} -name '*.jar' -exec ln -sv {}
"${lakehouse_iceberg_lib_dir}" \; -exec ln -sv {} "${iceberg_rest_lib_dir}" \;
Review Comment:
The `find` command for Iceberg bundles uses an unquoted variable (`find
${iceberg_bundle_dir} ...`) while other `find` usages in this script are
quoted. Quote it to avoid word-splitting/globbing issues and keep the style
consistent.
##########
dev/docker/gravitino/start-gravitino.sh:
##########
@@ -26,6 +26,42 @@ cd ${gravitino_dir}
python bin/rewrite_gravitino_server_config.py
+# Create soft links for JDBC drivers
+jdbc_driver_dir="${gravitino_dir}/jdbc-drivers"
+
+if [ -d "${jdbc_driver_dir}" ]; then
+ # Link MySQL driver to catalogs that need it
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-mysql/libs"
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-doris/libs"
+ mkdir -p "${gravitino_dir}/catalogs/jdbc-starrocks/libs"
+ mkdir -p "${gravitino_dir}/catalogs/lakehouse-iceberg/libs"
+ mkdir -p "${gravitino_dir}/iceberg-rest-server/libs"
+ mkdir -p "${gravitino_dir}/libs"
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-mysql/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-doris/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/jdbc-starrocks/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/catalogs/lakehouse-iceberg/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/iceberg-rest-server/libs/" \;
+ find "${jdbc_driver_dir}" -name "mysql-connector-java-*.jar" -exec ln -sfv
{} "${gravitino_dir}/libs/" \;
+
+ # Link PostgreSQL driver to catalogs that need it
+ find "${jdbc_driver_dir}" -name "postgresql-*.jar" -exec ln -sfv {}
"${gravitino_dir}/catalogs/jdbc-postgresql/libs/" \;
+ find "${jdbc_driver_dir}" -name "postgresql-*.jar" -exec ln -sfv {}
"${gravitino_dir}/catalogs/lakehouse-iceberg/libs/" \;
+ find "${jdbc_driver_dir}" -name "postgresql-*.jar" -exec ln -sfv {}
"${gravitino_dir}/iceberg-rest-server/libs/" \;
+ find "${jdbc_driver_dir}" -name "postgresql-*.jar" -exec ln -sfv {}
"${gravitino_dir}/libs/" \;
+fi
+
+# Create soft links for Iceberg bundle jars
+iceberg_bundle_dir="${gravitino_dir}/iceberg-bundles"
+lakehouse_iceberg_lib_dir="${gravitino_dir}/catalogs/lakehouse-iceberg/libs"
+iceberg_rest_lib_dir="${gravitino_dir}/iceberg-rest-server/libs"
+
+if [ -d "${iceberg_bundle_dir}" ]; then
+ mkdir -p "${lakehouse_iceberg_lib_dir}"
+ mkdir -p "${iceberg_rest_lib_dir}"
+ find ${iceberg_bundle_dir} -name '*.jar' -exec ln -sv {}
"${lakehouse_iceberg_lib_dir}" \; -exec ln -sv {} "${iceberg_rest_lib_dir}" \;
Review Comment:
`start-gravitino.sh` runs with `set -e`, but the Iceberg bundle linking uses
`ln -sv` (no `-f`). If the container restarts (or links already exist), `ln`
will fail and abort startup. Use a force/idempotent link creation (e.g., `ln
-sf...`) consistently, as done for the JDBC drivers.
##########
dev/docker/gravitino/gravitino-dependency.sh:
##########
@@ -64,22 +64,14 @@ mkdir -p "${gravitino_staging_dir}"
echo "Start to download the jar package"
-mysql_driver="mysql-connector-java-8.0.27.jar"
-wget
"https://repo1.maven.org/maven2/mysql/mysql-connector-java/8.0.27/$mysql_driver";
-O "${gravitino_staging_dir}/${mysql_driver}"
-cp "${gravitino_staging_dir}/${mysql_driver}"
"${gravitino_package_dir}/catalogs/jdbc-mysql/libs/"
-cp "${gravitino_staging_dir}/${mysql_driver}"
"${gravitino_package_dir}/catalogs/jdbc-doris/libs/"
-cp "${gravitino_staging_dir}/${mysql_driver}"
"${gravitino_package_dir}/catalogs/jdbc-starrocks/libs/"
-cp "${gravitino_staging_dir}/${mysql_driver}"
"${gravitino_package_dir}/catalogs/lakehouse-iceberg/libs/"
-cp "${gravitino_staging_dir}/${mysql_driver}" "${gravitino_iceberg_rest_dir}"
-cp "${gravitino_staging_dir}/${mysql_driver}" "${gravitino_package_dir}/libs/"
+jdbc_driver_dir="${gravitino_dir}/packages/gravitino/jdbc-drivers"
+mkdir -p "${jdbc_driver_dir}"
+mysql_driver="mysql-connector-java-8.0.27.jar"
+wget
"https://repo1.maven.org/maven2/mysql/mysql-connector-java/8.0.27/$mysql_driver";
-O "${jdbc_driver_dir}/${mysql_driver}"
Review Comment:
`jdbc_driver_dir` duplicates the already-defined `gravitino_package_dir`
prefix. Using `${gravitino_package_dir}/jdbc-drivers` avoids repeating the path
and reduces the chance of future inconsistencies if the package root changes.
--
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]
