[727] Fix demo notebook to run against current main - #850
Conversation
12ec54e to
124b0b0
Compare
vinishjail97
left a comment
There was a problem hiding this comment.
@rangareddy Can you validate if all demo notebooks are okay now?
|
Yes @vinishjail97 Executed the demo.ipynb notebook end-to-end after debugging and resolving several issues encountered during the runs. |
b4b9a61 to
574afcb
Compare
|
@rangareddy Thanks for working on this! I have validated the demo notebooks locally and added few comments on making the demo more robust. |
4ad2248 to
d6dd5fd
Compare
|
@vinishjail97 all five comments are addressed and answered inline. Force-pushed as a single commit ( The PR description needed a rewrite, not just an update: it documented a Two of your points found things worth calling out beyond the fix itself:
I also fixed one thing you did not raise but which sits in the same area: On verification, to be straight with you: I did not re-run the whole notebook this round. Cell 6 needs the Hive Metastore and I could not get one up locally — Three pre-existing things I noticed and deliberately did not change, each probably worth its own issue:
|
The demo had stopped working: the notebooks pinned dependency versions that no longer matched what xtable is built against, and the first cell provisioned them over the network on every kernel start. This makes the demo track the root pom, bakes the dependencies into the image, and removes the foot-guns that let it rot unnoticed in the first place. Demo runtime: - Derive every dependency version from the root pom. build_demo.sh writes demo/jars/versions.properties, and both the image's pre-fetch and the kernel predef read it, so there is one place a version is stated and the demo cannot silently drift from what xtable is tested against. - Bake JDK 11 and all notebook dependencies into the Jupyter image. Hudi 1.x requires Java 11+ while the almond base image only ships Java 8, and the coursier cache is warmed at build time so the first cell does not spend minutes resolving. - Move dependency loading out of the notebooks into a kernel predef, so the notebooks contain no dependency management. - Gate the Jupyter startup on the dependencies being resolvable, and re-fetch anything missing if versions changed since the image was built. - Run the demo in the background, split the jar build out into build_demo.sh, and add stop_demo.sh with --reset-data to restore the seed datasets. - Avoid an in-place Hudi table upgrade on the seed table, and make the Delta join cell re-runnable so the notebook can be executed more than once. - Create the demo namespace in the metastore automatically, qualifying the array construction as scala.Array: the notebook imports java.util._, so a bare Array(namespace) resolves against java.lang.reflect.Array in Ammonite. - Silence the harmless Presto RMI accept-loop warning via a log.properties. Hardening, from review on the PR: - Publish the notebook server on 127.0.0.1 only. It runs with authentication disabled, and 8888 was bound to every interface, so anyone able to reach the host had an unauthenticated Scala kernel: arbitrary code execution with read/write access to the mounted demo/data. Loopback keeps the printed http://localhost:8888/lab URL working. The other three services are left alone; they are pre-existing and not unauthenticated code execution. - Pin the base image to almondsh/almond:0.14.1 by tag and digest. The kernel-patching step depends on the base image's kernel path, on argv[0] being "java" and on -Xmx4g being safe to insert at argv[1], so a moving :latest could break the demo at build time - the same class of failure this change exists to fix. 0.14.1 is what :latest resolved to, same manifest digest, so behaviour is unchanged. - Assert in the kernel-patching step instead of silently doing nothing. With no kernel matched the glob returned an empty list, the comprehension iterated over nothing, and the build succeeded with an unpatched image that fails only when a notebook is run. Both the kernel path and the argv shape are checked. - Read the log4j, trino-jdbc and presto-jdbc versions from versions.properties like every other coordinate. These four were hardcoded in both prefetch_dependencies.sh and predef.sc, and a divergence between the two lists silently sends every kernel start to the network instead of the pre-warmed cache. log4j moves from 2.17.2 to the root pom's 2.22.0; 2.17.2 was carried over from the original demo (5d40c6f), not required by almond or Spark. - Fail in prefetch_dependencies.sh when a property is missing, rather than building a coordinate like "org.apache.spark:spark-sql_:" and surfacing it as a coursier resolution error. Values are read with cut -f2- so a value containing "=" is not truncated. - Remove stale xtable jars from demo/jars before copying new ones. predef.sc puts every jar in that directory on the kernel classpath, so after a version bump two copies of the same classes were loaded. Verified: image builds with the pinned base and patches all three Scala kernels; the kernel starts and reports log4j 2.22.0, Java 11 and Spark 3.4.2 with the xtable classes loaded; the notebook port is reachable on 127.0.0.1 and refused on the host's LAN address; both new asserts and the missing-property check fail loudly when their assumption is broken. Closes apache#727
252bf85 to
719ad4a
Compare
What is the purpose of the pull request
The demo notebook (
demo/notebook/demo.ipynb) no longer runs on currentmain. This PR fixes the issues reported in #727 plus several more found while getting it running end-to-end, and removes the foot-guns that let the demo rot unnoticed in the first place.It also silences the noisy Presto RMI warning reported in #316.
Closes #727
Closes #316
Changes
Notebook correctness —
demo/notebook/demo.ipynbPerTableConfigImpl(deleted in [297] Refactor config classes #480) to the currentSourceTable/TargetTable/ConversionConfigbuilders, andConversionSourceProvider#initto its single-Configurationsignature. Explicit emptyPropertiesare passed on the builders becauseHudiSourceConfig.fromPropertiesthrows an NPE whenadditionalPropertiesis null.hoodie.write.schemaoverride: under Hudi 1.2's lazy record serialization the rows are encoded with the nullable dataframe-derived schema but were decoded with the non-null override schema, so every record failed withAvroRuntimeException: Malformed data. Length is negative. With the override removed one consistent schema is used.scala.Array(namespace)— the notebook importsjava.util._, so a bareArray(namespace)resolves againstjava.lang.reflect.Arrayunder Ammonite and fails to compile.Versions come from the root pom —
demo/build_demo.sh(new),demo/jupyter/predef.sc(new)The root cause of #727 is that the demo stated its own versions.
build_demo.shnow builds the demo jars and writesdemo/jars/versions.propertiesfrom the root pom, and both the image's dependency pre-fetch and the kernel predef read that file. There is one place a version is stated, so the demo cannot silently drift from what xtable is built against.Covers
scala.binary.version,spark.version,spark.version.prefix,hudi.version,delta.version,delta.kernel.version,iceberg.hive.runtime.version,log4j.version, plustrino.jdbc.version/presto.jdbc.version(literals inbuild_demo.sh, since xtable does not depend on the JDBC drivers).delta-kernel-api/delta-kernel-defaultsare included because they areprovided-scope dependencies of xtable-core, and without them theConversionTargetServiceLoader fails onDeltaKernelConversionTargetbefore any sync can run.Dependencies baked into the image —
demo/jupyter/Dockerfile(new)Hudi 1.x classes are Java 11 bytecode but
almondsh/almondonly ships Java 8 (there is no JDK 11 tag), which failed withUnsupportedClassVersionError. The image now installs OpenJDK 11, pre-fetches every notebook dependency into the coursier cache at build time, and re-points the Scala kernels at that JDK with a bounded heap and the predef wired in — so the first cell does not spend minutes provisioning, and there is no one-time download on first start.demo/jupyter/verify_dependencies.sh(new) re-checks the cache before Jupyter starts and fetches anything missing, so a version bump without an image rebuild still works.Demo lifecycle —
demo/start_demo.sh,demo/stop_demo.sh(new)The demo runs in the background, the jar build is split out into
build_demo.sh, andstop_demo.sh --reset-datarestores the seed datasets.Presto log noise —
demo/presto/log.properties(new) — fixes #316The presto container repeatedly logged
WARN RMI TCP Accept-0 sun.rmi.transport.tcp … accept loop for ServerSocket[…] throws java.net.SocketTimeoutException. The JMX agent's RMI accept loop emits it whenever itsaccept()times out — harmless, but it floods the compose logs and reads like a startup failure. The newlog.propertiesraises that logger toERRORand is mounted atetc/log.properties, which the presto entrypoint already passes as-Dlog.levels-file.Hardening added in review
Thanks @vinishjail97 — all five points are addressed, each with its own reply on the thread.
127.0.0.1only. It runs with authentication disabled and8888was bound to every interface, so anyone able to reach the host had an unauthenticated Scala kernel: arbitrary code execution with read/write access to the mounteddemo/data. The other three services are unchanged — pre-existing, and not unauthenticated code execution.almondsh/almond:0.14.1@sha256:0407359b…). The kernel-patching step depends on the base image's kernel path, onargv[0]beingjava, and on-Xmx4gbeing safe to insert atargv[1], so a moving:latestcould break the demo at build time — the same class of failure this PR exists to fix.0.14.1is what:latestresolved to (identical manifest digest), so this changes nothing today.argvshape are now checked.prefetch_dependencies.shfails on a missing property rather than building a coordinate likeorg.apache.spark:spark-sql_:and surfacing it as a coursier resolution error. Values are read withcut -f2-so a value containing=is not truncated.5d40c6f), not required by almond or Spark.demo/jarsbefore copying new ones.predef.scputs every jar in that directory on the kernel classpath, so after a version bump two copies of the same classes were loaded.Verification
What does still pass, on the current revision:
For #316: recreated the presto container with the new mount and confirmed the server reaches
======== SERVER STARTED ========with all four catalogs loaded and nosun.rmi.transport.tcplines indocker logs presto.Hardening round — the notebook cells are untouched by these changes, so rather than a full re-run I verified each changed mechanism directly:
docker port jupyter127.0.0.1:8888; loopback → HTTP 200, host LAN address → refused;trinostill0.0.0.0for contrastpatched kernels: [scala212, scala213, scala36]XTable demo kernel ready: …org.apache.xtable.model.InternalTableresolvingno scala kernels found…,kernel argv[0] is not java: ['python'])prop()with a missing key./demo/build_demo.shBUILD SUCCESS, all ten properties emittedTo be explicit: I did not re-run the whole notebook in this round. Cell 6 needs the Hive Metastore, and a local HMS could not be brought up on this machine —
apache/hive:4.0.0-alpha-2is amd64-only and sat in emulated derbyinitOrUpgradeSchemawithout progressing. A confirming end-to-end run on x86 would be welcome before merge.Known failure — incremental Hudi → Iceberg sync on pristine data
The round-2 append cell fails, deterministically, on a freshly reset
demo/data:Evidence chain:
CustomerKey20231105172136406.commit"string"→ requiredidentifier-field-ids=[2],required=true["null","string"]→ optionalThe round-2 CSV is read with
header=trueand no schema, so Spark types every column as a nullable string, flippingCustomerKeyfrom required to optional.IcebergSchemaExtractor.toIcebergthen correctly declines to set identifier fields for a nullable record key (the guard added for #366), so XTable's target schema has none — but the table still carriesidentifier-field-ids=[2]from the initial sync, andSchemaUpdate.apply()re-validates those against the new schema.grep -rn 'setIdentifierFields' xtable-core/src/main/javareturns nothing:IcebergSchemaSyncnever reconciles identifier fields, so once a record key goes required → optional the Iceberg target is permanently wedged.This is a distinct gap from #261 / #366, which fixed table creation with an already-nullable key, not the transition on an existing table. It reproduces outside the demo and is an XTable bug, not a demo bug — but the demo is where it surfaces, because the seed's required key plus a schema-less round-2 read makes it deterministic, and
stop_demo.sh --reset-datarestores exactly that state.How XTable should behave is a semantics call (drop the identifier fields, refuse the nullability change, or hold the key required), so it needs a decision rather than a quick patch. Filing separately; this PR should probably not be merged claiming a working demo until it is resolved, since the round-2 cell is part of the notebook.
Note that
inferSchema=trueon the round-2 read is not a fix: the existing schema is all required strings, so inference additionally yieldsCustomerKey: intandYearlyIncome: double, adding a string → int change that Iceberg rejects outright.Notes for reviewers
almondsh/almondpublishes amd64 only, so the whole demo runs under emulation on Apple Silicon and is noticeably slower there. Pre-existing, not changed here.docker-compose.yamlruns trino 428 while the driver is trino-jdbc 431 (presto matches at 0.283). Left alone since the driver is version-tolerant and the notebooks were validated on it, but both now live in one place if you want them aligned.node.id=prestoto the trackeddemo/presto/node.properties, so running the demo dirties the working tree. Pre-existing; worth a separate fix.container_name: hive-metastoreis a hardcoded global name, so the demo collides with any other local compose project using it. Pre-existing; worth a separate fix.Verify this pull request
No automated test coverage — this is demo tooling. Reviewers should run
cd demo && ./start_demo.shand executedemo/notebook/demo.ipynb.