rangareddy commented on code in PR #850:
URL: https://github.com/apache/incubator-xtable/pull/850#discussion_r3718505365
##########
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:
Fixed — bound to loopback, as you suggested:
```yaml
ports:
- '127.0.0.1:8888:8888'
```
I took this over the keep-the-token alternative: it removes the reachability
outright rather than relying on a secret, adds no new failure mode, and
`http://localhost:8888/lab` keeps working exactly as `start_demo.sh` already
prints it. I also rewrote the comment so it says what the binding is protecting
rather than only stating intent — the previous `## Local demo only` was the
thing that wasn't enforcing anything.
Verified against the running container rather than just reading the config:
```
$ docker port jupyter
8888/tcp -> 127.0.0.1:8888
$ netstat -an | grep '\.8888'
tcp4 0 0 127.0.0.1.8888 *.* LISTEN
$ curl -o /dev/null -w '%{http_code}\n' http://127.0.0.1:8888/lab
200
$ curl -o /dev/null -w '%{http_code}\n' --max-time 6
http://10.3.232.82:8888/lab # host LAN address
000
# refused
$ docker port trino # untouched, for contrast
8080/tcp -> 0.0.0.0:8080
```
Left the other three services as they are, per your note that they 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:
Both done.
**Pinned by tag *and* digest**, since a tag can be re-pushed:
```dockerfile
FROM
almondsh/almond:0.14.1@sha256:0407359be58529eaa8e35bcf4586c3a890123be64005600b381de88fdc3c5de7
```
Worth noting this pin is a **no-op behaviourally**: Docker Hub reports
`latest` and `0.14.1` with the *same* manifest digest (`sha256:0407359be5…`,
last updated 2025-02-25). So it changes nothing today and just stops the tag
moving underneath us.
One correction to your suggestion: `0.14.0-scala-2.12.17` does not exist.
The scala-suffixed tags are per-Scala *single-kernel* images
(`0.14.1-scala-2.12.20`, `-2.13.16`, `-3.6.3`); the unsuffixed `0.14.1` is the
multi-kernel image that the `scala*` glob in this file is written for, so that
is the correct one to pin.
**Asserts** — I added two rather than one, because you named three
assumptions and `assert paths` only covers the first:
```python
paths = sorted(glob.glob('.../kernels/scala*/kernel.json'))
assert paths, 'no scala kernels found under
/home/jovyan/.local/share/jupyter/kernels'
kernels = [(p, json.load(open(p))) for p in paths]
assert all(k['argv'][0] == 'java' for _, k in kernels), 'kernel argv[0] is
not java: %r' % [...]
```
If `argv[0]` were ever a wrapper script rather than the JVM, replacing index
0 and inserting `-Xmx4g` at index 1 would produce a broken kernel that the glob
assert would happily pass. `sorted()` also makes the patch order deterministic.
Verified the assumptions actually hold for the pinned image:
```
/home/jovyan/.local/share/jupyter/kernels/{scala212,scala213,scala36}/
argv[0] == "java", user == jovyan, base java == 1.8.0_442 (so the JDK 11
install is indeed needed)
```
and that the asserts fail rather than no-op when broken:
```
AssertionError: no scala kernels found under
/home/jovyan/.local/share/jupyter/kernels
AssertionError: kernel argv[0] is not java: ['python']
```
The real build then reports `patched kernels: [scala212, scala213, scala36]`.
One thing you may want to know separately: `almondsh/almond` publishes
**amd64 only** — there is no arm64 variant, so on Apple Silicon the whole demo
runs under emulation. Pre-existing, but it makes the demo noticeably slow on
M-series machines, and it is why I could not get a throwaway Hive Metastore
through derby schema init locally.
##########
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:
Applied exactly as written, including `cut -d= -f2-`.
Verified both halves:
```
$ bash demo/jupyter/prefetch_dependencies.sh <props file missing
log4j.version>
ERROR: log4j.version missing from …/props-incomplete.properties; re-run
build_demo.sh
# and the -f2- fix
weird.key -> a=b=c (old cut -f2 gave: a)
```
This turned out to pay for itself immediately: because the change below adds
three new keys, a stale `demo/jars/versions.properties` from a build predating
it now stops the container at the startup gate naming the missing key, instead
of failing later inside coursier.
##########
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:
Done — all four now come from `versions.properties`:
- `build_demo.sh` adds `log4j.version` to the pom-property loop, and
declares `TRINO_JDBC_VERSION` / `PRESTO_JDBC_VERSION` as literals with a
comment tying them to the trino/presto image tags in `docker-compose.yaml` (as
you suggested — there is no root-pom property because xtable does not depend on
either driver).
- `prefetch_dependencies.sh` and `predef.sc` both read all three.
- Both lists now carry a comment explaining they must resolve to the same
artifacts, and why: cache-warm at build time vs resolve at kernel start.
No coordinate in either file states a version any more.
`verify_dependencies.sh` needed no change since it delegates to
`prefetch_dependencies.sh`.
`./demo/build_demo.sh` → `BUILD SUCCESS`, and `versions.properties` now has
all ten keys:
```
scala.binary.version=2.12 delta.kernel.version=4.0.0
spark.version=3.4.2 iceberg.hive.runtime.version=1.7.2
spark.version.prefix=3.4 log4j.version=2.22.0
hudi.version=1.2.0 trino.jdbc.version=431
delta.version=2.4.0 presto.jdbc.version=0.283
```
One thing I noticed once they were side by side and did **not** change:
`docker-compose.yaml` runs **trino 428** while the driver is **trino-jdbc 431**
(presto matches at 0.283). The Trino JDBC driver is version-tolerant and you
validated the notebooks on 431, so I left it — but they are now in one place if
you want them aligned.
##########
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:
Carried over — not intentional, and nothing in the almond/Spark stack needs
it.
`git log -S'2.17.2' -- demo/` traces it to **`5d40c6f` "Add Demo for OSDS
(#173)"**, the original demo commit, and the pre-PR notebook has the same two
coordinates inline:
```
log4j:log4j-api:2.17.2
log4j:log4j-core:2.17.2
```
2.17.2 is the post-Log4Shell release from early 2022, i.e. simply what was
current when the demo was first written.
So it now reads `log4j.version` from `versions.properties`, which
`build_demo.sh` takes from the root pom — the demo tracks
`<log4j.version>2.22.0</log4j.version>` and will not drift again on the next
bump.
Since this is the one change here with actual runtime risk (log4j sits on
the Spark classpath), I checked it in a running kernel rather than just
building:
```
log4j on classpath: 2.22.0
java: 11.0.30-internal
spark: 3.4.2
xtable model: org.apache.xtable.model.InternalTable
```
with the predef compiling clean and printing `XTable demo kernel ready: …`.
--
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]
