vinishjail97 commented on code in PR #850: URL: https://github.com/apache/incubator-xtable/pull/850#discussion_r3716212904
########## demo/jupyter/prefetch_dependencies.sh: ########## @@ -0,0 +1,52 @@ +#!/bin/bash +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +## Pre-fetch the demo notebook dependencies into the coursier cache at image +## build time so the notebooks' first cell resolves instantly. The dependency +## versions are read from the versions.properties file passed as the first +## argument (generated from the root pom by build_demo.sh). +set -e + +PROPS_FILE="$1" + +prop() { + grep "^$1=" "${PROPS_FILE}" | cut -d= -f2 +} + +SCALA_BINARY_VERSION=$(prop scala.binary.version) +SPARK_VERSION=$(prop spark.version) Review Comment: `prop()` returns an empty string for a missing or renamed key, with no error. So a typo — or `build_demo.sh` dropping a property from its loop at line 49 — produces `org.apache.spark:spark-sql_:` and a coursier resolution error that says nothing about the real cause. Since `set -e` is on, the image build fails at that point with a confusing message. `predef.sc:33-36` gets this right (`sys.error(s"$key missing from ...")`). Mirror it: ```bash prop() { local value value=$(grep "^$1=" "${PROPS_FILE}" | cut -d= -f2-) if [ -z "${value}" ]; then echo "ERROR: $1 missing from ${PROPS_FILE}; re-run build_demo.sh" >&2 exit 1 fi echo "${value}" } ``` Note `-f2-` rather than `-f2` — the current version truncates any value containing `=`. None do today, but it is free to be correct. ########## demo/jupyter/predef.sc: ########## @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +// Kernel predef for the XTable demo notebooks: loads every dependency the +// notebooks need when the kernel starts, so the notebooks themselves contain +// no dependency management. The artifacts are already present in the image's +// coursier cache (pre-fetched at image build time); versions come from +// /home/jars/versions.properties, generated from the root pom by +// build_demo.sh. + +val xtableVersions = { + val props = new java.util.Properties() + val in = new java.io.FileInputStream("/home/jars/versions.properties") + try props.load(in) + finally in.close() + props +} + +def xtableVersion(key: String): String = + Option(xtableVersions.getProperty(key)).map(_.trim).filter(_.nonEmpty).getOrElse { + sys.error(s"$key missing from /home/jars/versions.properties, re-run build_demo.sh") + } + +{ + val scalaBinaryVersion = xtableVersion("scala.binary.version") + val sparkVersionPrefix = xtableVersion("spark.version.prefix") + + Seq( + ("org.apache.logging.log4j", "log4j-api", "2.17.2"), Review Comment: log4j is pinned to 2.17.2 here while the project itself builds against 2.22.0 (`org.apache.logging.log4j:log4j-api:2.22.0` in the shaded modules' `LICENSE-bundled`). Given the surrounding change deliberately moves Hudi and Iceberg onto the versions the root pom pins, was the older log4j intentional — something in the almond/Spark stack that needs it — or just carried over from the original notebook? If there is no reason for it, reading `log4j.version` from `versions.properties` like everything else keeps the demo consistent with what XTable is actually tested against. ########## demo/docker-compose.yaml: ########## @@ -54,7 +55,12 @@ services: jupyter: container_name: jupyter hostname: jupyter - image: 'almondsh/almond:latest' + build: + context: . + dockerfile: jupyter/Dockerfile + image: 'xtable-demo-jupyter' + ## Local demo only: start the notebook server without authentication. + command: ["start-notebook.sh", "--IdentityProvider.token=", "--ServerApp.password="] Review Comment: This disables authentication on the notebook server, and line 65 publishes it as `'8888:8888'` — bound to every interface on the host. Combined, anyone who can reach the machine on port 8888 (same office wifi, same k8s node, same VPN segment) gets an unauthenticated Scala kernel with arbitrary code execution and read/write access to the mounted `demo/data`. The `## Local demo only` comment states the intent but nothing enforces it, and the previous behaviour (token in `docker logs`) was safe by default. At minimum bind to loopback: ```yaml ports: - '127.0.0.1:8888:8888' ``` That keeps the zero-friction "open the printed URL" experience the PR is going for — `start_demo.sh` already prints `http://localhost:8888/lab` — while making the exposure actually local. If you would rather keep the token, `start_demo.sh` could grep it out of `docker logs` and print the full URL, which is the same UX and needs no auth change at all. Worth noting the other three services (lines 22, 32, 47) have the same all-interfaces binding, but those are pre-existing and not unauthenticated code execution. ########## demo/jupyter/Dockerfile: ########## @@ -0,0 +1,52 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Jupyter image for the XTable demo, based on the almond Scala kernel image. +# Everything the notebooks need at runtime is baked in at build time so the +# first cell does not spend minutes provisioning: +# - OpenJDK 11 for the Scala kernels (Hudi 1.x jars require Java 11+) +# - all notebook dependencies (Spark, Hudi, Delta, Iceberg, JDBC drivers) +# pre-fetched into the coursier cache +# - the Scala kernels re-pointed at that JDK, with a bounded heap and a +# predef that puts every dependency on the classpath at kernel startup +FROM almondsh/almond:latest Review Comment: `:latest` on the base image, in a PR whose whole purpose is that the demo stopped working because things moved underneath it. That matters more than usual here because of the kernel-patching step at the bottom of this file: it globs `/home/jovyan/.local/share/jupyter/kernels/scala*/kernel.json`, replaces `argv[0]` positionally, and inserts `-Xmx4g` at index 1. All three assumptions belong to whatever `almondsh/almond:latest` is on the day the image is built. When the tag moves — a different kernel path, a different `argv` shape, a jovyan→different user change — the glob matches nothing, the `for` comprehension iterates over an empty list, `print('patched kernels:', [])` succeeds, and the build passes with a broken image. Someone will then reopen #727. Pin it. `almondsh/almond` publishes versioned tags (`0.14.0-scala-2.12.17` and similar); a digest pin (`FROM almondsh/almond@sha256:...`) is stronger still. And make the patch step assert rather than no-op: ```python assert paths, 'no scala kernels found under ~/.local/share/jupyter/kernels' ``` Same reasoning applies to `image: 'xtable-demo-jupyter'` in the compose file — an untagged local image means `:latest`, so a stale build from a previous checkout gets reused silently unless `--build` is passed (which `start_demo.sh` does, so that one is fine in practice). ########## demo/jupyter/prefetch_dependencies.sh: ########## @@ -0,0 +1,52 @@ +#!/bin/bash +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +## Pre-fetch the demo notebook dependencies into the coursier cache at image +## build time so the notebooks' first cell resolves instantly. The dependency +## versions are read from the versions.properties file passed as the first +## argument (generated from the root pom by build_demo.sh). +set -e + +PROPS_FILE="$1" + +prop() { + grep "^$1=" "${PROPS_FILE}" | cut -d= -f2 +} + +SCALA_BINARY_VERSION=$(prop scala.binary.version) +SPARK_VERSION=$(prop spark.version) +SPARK_VERSION_PREFIX=$(prop spark.version.prefix) +HUDI_VERSION=$(prop hudi.version) +DELTA_VERSION=$(prop delta.version) +DELTA_KERNEL_VERSION=$(prop delta.kernel.version) +ICEBERG_HIVE_RUNTIME_VERSION=$(prop iceberg.hive.runtime.version) + +coursier fetch --quiet \ + "org.apache.logging.log4j:log4j-api:2.17.2" \ Review Comment: These four coordinates — log4j 2.17.2 (×2), `io.trino:trino-jdbc:431`, `com.facebook.presto:presto-jdbc:0.283` — are hardcoded here *and* again in `predef.sc:44-55`. That is the exact duplication `versions.properties` was introduced to eliminate; the other eight coordinates correctly read from it, these four do not. The drift is silent and has a real cost. The prefetch list is what warms the coursier cache at image build time; the predef list is what the kernel resolves at startup. If they diverge by one version, the predef falls through to a network resolve on every kernel start — which is precisely the slow path the Dockerfile comment says this design exists to avoid ("so the first cell does not spend minutes provisioning"), and it will look like a flaky demo rather than a version mismatch. Put them in `versions.properties` alongside the rest. `build_demo.sh:49` already loops over a property list, so it is a matter of adding `log4j.version`, `trino.jdbc.version` and `presto.jdbc.version` — the first can come from the root pom like the others, and the last two can be literals in `build_demo.sh` if there is no pom property for them. Either way there is then one place to change a version. -- 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]
