Skip to content

refactor(notebook-migration-service): compute jupyter iframe url per request - #7602

Open
zyratlo wants to merge 3 commits into
apache:mainfrom
zyratlo:migration-tool-stateless-backend
Open

refactor(notebook-migration-service): compute jupyter iframe url per request#7602
zyratlo wants to merge 3 commits into
apache:mainfrom
zyratlo:migration-tool-stateless-backend

Conversation

@zyratlo

@zyratlo zyratlo commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

Makes notebook-migration-service stateless so it can later run as a single global instance instead of one instance per user. This is the first backend stage of moving the service onto Texera's "orchestrator services are global, stateful resources are per user" pattern.

Today the service keeps a shared @volatile jupyterIframeURL: set-notebook writes it and get-jupyter-iframe-url reads it back. That shared state is only safe because each user happens to run their own pod, and even within one user it lets two browser tabs race. This PR removes the shared state and builds the URL from the request instead.

NotebookMigrationResource.scala

  • Removes the @volatile var jupyterIframeURL singleton and the warning comment that documented its per-user-pod assumption. Adds a defaultNotebookName constant (notebook.ipynb).
  • getJupyterIframeURL now takes a notebookName argument and builds the URL on each call. The name is validated with the same plain .ipynb regex setNotebook uses, since it now flows straight into the returned URL (blocks path traversal). The argument defaults to defaultNotebookName.
  • setNotebook no longer mutates any shared state; the assignment that wrote the singleton is gone. Its upload behavior is unchanged.
  • The /get-jupyter-iframe-url endpoint accepts an optional notebookName query parameter and falls back to the default when it is absent.

The change is backward compatible. The existing frontend calls the endpoint with no query parameter, which resolves to notebook.ipynb, exactly the URL it received before. No frontend, config, or deployment change is needed in this PR, and no other service or branch consumes the removed state.

Any related issues, documentation, discussions?

Closes #7390
Parent-issue #4301

How was this PR tested?

Extends the existing suite in NotebookMigrationResourceSpec.scala:

  • Fixed the two call sites that pass through the new endpoint signature.
  • Added a test that an explicit notebookName is honored in the returned URL.
  • Added a test that an invalid notebookName is rejected with 400 before any Jupyter call.
  • Added a test pinning the refactor: after setNotebook uploads other.ipynb, a parameter-less getJupyterIframeURL returns the default notebook.ipynb, proving the result no longer depends on state left by setNotebook.

Was this PR authored or co-authored using generative AI tooling?

Generated-by: Claude Code (Claude Opus 4.8)

@github-actions github-actions Bot added the platform Non-amber Scala service paths label Aug 12, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • No candidates found from git blame history.

@codecov-commenter

codecov-commenter commented Aug 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.39%. Comparing base (eefd860) to head (96889a6).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #7602      +/-   ##
============================================
- Coverage     89.62%   89.39%   -0.24%     
+ Complexity     4395     4375      -20     
============================================
  Files          1177     1177              
  Lines         46884    46835      -49     
  Branches       5239     5225      -14     
============================================
- Hits          42022    41868     -154     
- Misses         3109     3220     +111     
+ Partials       1753     1747       -6     
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø)
agent-service 98.62% <ø> (ø) Carriedforward from 052cf38
amber 85.74% <ø> (-0.64%) ⬇️ Carriedforward from 052cf38
computing-unit-managing-service 72.46% <ø> (ø)
config-service 77.31% <ø> (ø)
file-service 68.90% <ø> (ø)
frontend 90.57% <ø> (-0.02%) ⬇️ Carriedforward from 052cf38
notebook-migration-service 79.31% <100.00%> (+0.41%) ⬆️
pyamber 97.57% <ø> (ø) Carriedforward from 052cf38
workflow-compiling-service 57.89% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@zyratlo
zyratlo marked this pull request as ready for review August 13, 2026 17:53
@zyratlo

zyratlo commented Aug 13, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @mengw15

@github-actions
github-actions Bot requested a review from mengw15 August 13, 2026 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform Non-amber Scala service paths

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Notebook Migration] Refactor notebook-migration-service to be stateless

2 participants