rangareddy commented on PR #850: URL: https://github.com/apache/incubator-xtable/pull/850#issuecomment-5188514842
@vinishjail97 all five comments are addressed and answered inline. Force-pushed as a **single commit** (`d6dd5fd`) — the 19 commits are squashed, and I verified the squash is byte-identical to the previous diff (same `git diff` checksum against the merge base) so nothing was lost or reordered in content. The PR description needed a rewrite, not just an update: it documented a `demo/jupyter/setup-jdk11.sh` startup hook and inline notebook version pins, neither of which exists any more — JDK 11 and every dependency are baked into the image and all versions come from the root pom. It also named `xtable-core_2.12-0.2.0-SNAPSHOT.jar` explicitly, which the predef no longer does. There is a note at the top of the description flagging what changed, so anyone arriving mid-thread is not misled. Two of your points found things worth calling out beyond the fix itself: - **The `:latest` pin was hiding three assumptions, not one.** I added a second assert on `argv[0] == 'java'` alongside `assert paths`: 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 passes happily. Both fire correctly on negative tests. Also, `0.14.0-scala-2.12.17` does not exist — the scala-suffixed tags are single-kernel images, so the unsuffixed `0.14.1` is the right one for the `scala*` glob. It has the same manifest digest as `latest`, so the pin changes nothing today. - **log4j 2.17.2 was carried over, not deliberate** — `git log -S'2.17.2'` traces it to `5d40c6f`, the original demo commit, and the pre-PR notebook has the same coordinates inline. Now tracks the root pom's 2.22.0, verified as 2.22.0 on a live kernel classpath. I also fixed one thing you did not raise but which sits in the same area: `build_demo.sh` never cleared `demo/jars`, and `predef.sc` puts **every** jar there on the kernel classpath. After this branch picked up main's `0.5.0-SNAPSHOT` bump, my local `demo/jars` held both 0.2.0 and 0.5.0 copies of all three jars — two copies of the same classes, resolved in filesystem order. Added `rm -f demo/jars/xtable-*.jar` before the copies. **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 — `apache/hive:4.0.0-alpha-2` is amd64-only and sat in emulated derby `initOrUpgradeSchema` without progressing on Apple Silicon. What I did verify is every mechanism these changes touch: the image builds with the pinned base and patches all three Scala kernels; the kernel starts, the predef compiles, and it reports log4j 2.22.0 / Java 11 / Spark 3.4.2 with `org.apache.xtable.model.InternalTable` resolving; `docker port` shows `127.0.0.1:8888` with the host's LAN address refused; and both asserts plus the missing-property check fail loudly when broken. The notebook cells themselves are untouched by this round. A confirming end-to-end run on x86 before merge would be welcome. Three pre-existing things I noticed and deliberately did not change, each probably worth its own issue: 1. `almondsh/almond` is **amd64 only**, so the whole demo runs emulated on Apple Silicon. 2. Starting presto **appends `node.id=presto`** to the tracked `demo/presto/node.properties`, so running the demo dirties the working tree. 3. `container_name: hive-metastore` is a hardcoded global name, so the demo collides with any other local compose project using that name. -- 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]
