You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "zyratlo (via GitHub)" <gi...@apache.org> on 2026/08/12 22:42:20 UTC
[PR] refactor(notebook-migration-service): compute jupyter iframe url per request [texera]
zyratlo opened a new pull request, #7602:
URL: https://github.com/apache/texera/pull/7602
### 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)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7602:
URL: https://github.com/apache/texera/pull/7602#issuecomment-5273655744
## [Codecov](https://app.codecov.io/gh/apache/texera/pull/7602?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `90.00000%` with `1 line` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 88.63%. Comparing base ([`71faf44`](https://app.codecov.io/gh/apache/texera/commit/71faf440de7936d07451d214dbd5777e6906b949?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d2b4c3b`](https://app.codecov.io/gh/apache/texera/commit/d2b4c3bc79152e647a75ddd749cbe3025419f245?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/texera/pull/7602?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...a/service/resource/NotebookMigrationResource.scala](https://app.codecov.io/gh/apache/texera/pull/7602?src=pr&el=tree&filepath=notebook-migration-service%2Fsrc%2Fmain%2Fscala%2Forg%2Fapache%2Ftexera%2Fservice%2Fresource%2FNotebookMigrationResource.scala&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-bm90ZWJvb2stbWlncmF0aW9uLXNlcnZpY2Uvc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS90ZXhlcmEvc2VydmljZS9yZXNvdXJjZS9Ob3RlYm9va01pZ3JhdGlvblJlc291cmNlLnNjYWxh) | 90.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/texera/pull/7602?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #7602 +/- ##
============================================
- Coverage 88.63% 88.63% -0.01%
Complexity 4346 4346
============================================
Files 1177 1177
Lines 46896 46901 +5
Branches 5230 5231 +1
============================================
+ Hits 41567 41571 +4
- Misses 3568 3569 +1
Partials 1761 1761
```
| [Flag](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | | *Carryforward flag |
|---|---|---|---|
| [access-control-service](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `70.00% <ø> (ø)` | | |
| [agent-service](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `98.62% <ø> (ø)` | | Carriedforward from [71faf44](https://app.codecov.io/gh/apache/texera/commit/71faf440de7936d07451d214dbd5777e6906b949?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [amber](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `84.71% <ø> (ø)` | | Carriedforward from [71faf44](https://app.codecov.io/gh/apache/texera/commit/71faf440de7936d07451d214dbd5777e6906b949?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [computing-unit-managing-service](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `60.38% <ø> (ø)` | | |
| [config-service](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `77.31% <ø> (ø)` | | |
| [file-service](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.05% <ø> (ø)` | | Carriedforward from [71faf44](https://app.codecov.io/gh/apache/texera/commit/71faf440de7936d07451d214dbd5777e6906b949?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [frontend](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `89.98% <ø> (ø)` | | Carriedforward from [71faf44](https://app.codecov.io/gh/apache/texera/commit/71faf440de7936d07451d214dbd5777e6906b949?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [notebook-migration-service](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `78.92% <90.00%> (+0.02%)` | :arrow_up: | |
| [pyamber](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `97.52% <ø> (ø)` | | Carriedforward from [71faf44](https://app.codecov.io/gh/apache/texera/commit/71faf440de7936d07451d214dbd5777e6906b949?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [workflow-compiling-service](https://app.codecov.io/gh/apache/texera/pull/7602/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `57.89% <ø> (ø)` | | |
*This pull request uses carry forward flags. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/texera/pull/7602?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3786621750
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -457,9 +463,15 @@ class NotebookMigrationResource extends LazyLogging {
@GET
@Path("/get-jupyter-iframe-url")
- def getJupyterIframeURL(@Auth user: SessionUser): Response = {
+ def getJupyterIframeURL(
+ @QueryParam("notebookName") notebookName: String,
+ @Auth user: SessionUser
+ ): Response = {
logger.info("Getting Jupyter iframe URL")
- NotebookMigrationResource.getJupyterIframeURL()
+ val name = Option(notebookName)
+ .filter(_.nonEmpty)
+ .getOrElse(NotebookMigrationResource.defaultNotebookName)
Review Comment:
Since modifying the frontend portion depends on #7601, I will make these changes once that is merged
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3781283499
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -104,8 +101,17 @@ object NotebookMigrationResource extends LazyLogging {
}
}
- // Returns the Jupyter iframe reference URL
- def getJupyterIframeURL(): Response = {
+ // Returns the Jupyter iframe reference URL for the given notebook.
+ def getJupyterIframeURL(notebookName: String): Response = {
+ // notebookName flows into the returned URL, so validate it the same way setNotebook does:
Review Comment:
This mirrors setNotebook's validation, but the comment over there still justifies itself partly with "keeps notebookName out of the raw-interpolated jupyterIframeURL JSON" — and `jupyterIframeURL` is gone as of this PR. Same stale-reference class Copilot flagged on the frontend. Its path-traversal half still stands; worth fixing the other half while it's in scope.
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -75,13 +75,10 @@ object NotebookMigrationResource extends LazyLogging {
private val jupyterUrl = StorageConfig.jupyterURL
private val jupyterToken = StorageConfig.jupyterToken
- // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
- // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
- // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
- // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
- // per-user keying here, or one user's upload would overwrite another's iframe URL.
- @volatile private var jupyterIframeURL =
- s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ // Default notebook name used when a request does not specify one, so a param-less
+ // getJupyterIframeURL call reproduces the URL from before this service became stateless.
+ private val defaultNotebookName = "notebook.ipynb"
Review Comment:
The block being removed also carried a deployment warning — "Do NOT deploy this service as a shared multi-user instance without adding per-user keying here". Removing the state removes one reason for it, but not the hazard: `jupyterUrl` and `jupyterToken` are still single process-wide values, so a shared instance would hand every user the same Jupyter and the same token.
Since this is explicitly stage 1, worth keeping a line to that effect so the constraint survives to the stage that actually lifts it.
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -457,9 +463,15 @@ class NotebookMigrationResource extends LazyLogging {
@GET
@Path("/get-jupyter-iframe-url")
- def getJupyterIframeURL(@Auth user: SessionUser): Response = {
+ def getJupyterIframeURL(
+ @QueryParam("notebookName") notebookName: String,
+ @Auth user: SessionUser
+ ): Response = {
logger.info("Getting Jupyter iframe URL")
- NotebookMigrationResource.getJupyterIframeURL()
+ val name = Option(notebookName)
+ .filter(_.nonEmpty)
+ .getOrElse(NotebookMigrationResource.defaultNotebookName)
Review Comment:
This fallback is only reachable because the frontend still sends a fixed `notebook.ipynb` for every workflow. Its comment there justifies that with "each user runs their own pod, so a single notebook.ipynb never collides" — true across users, but not across one user's workflows: they all land on the same `work/notebook.ipynb`, so switching workflows overwrites it, and since nothing writes back from Jupyter, edits made in the panel are gone. Two tabs on different workflows likewise end up on the same file while each keeps its own cell-highlight mapping.
This PR is what makes the fix possible — `notebook_<wid>.ipynb` already passes the regex and needs nothing further from the backend.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3786607083
##########
notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala:
##########
@@ -481,9 +481,42 @@ class NotebookMigrationResourceSpec
urlResp.getStatus shouldBe Response.Status.OK.getStatusCode
urlResp.getEntity.toString should include("localhost:9100")
- val iframeResp = resource.getJupyterIframeURL(sessionUser(writerUid))
+ val iframeResp = resource.getJupyterIframeURL(null, sessionUser(writerUid))
iframeResp.getStatus shouldBe Response.Status.OK.getStatusCode
- iframeResp.getEntity.toString should include("/notebooks/work/")
+ iframeResp.getEntity.toString should include("/notebooks/work/notebook.ipynb")
+ }
+ }
+
+ it should "build the iframe URL from an explicit notebook name" in {
+ withFakeJupyter(contentsStatus = 201) {
+ val resp = NotebookMigrationResource.getJupyterIframeURL("other.ipynb")
+ resp.getStatus shouldBe Response.Status.OK.getStatusCode
+ resp.getEntity.toString should include("/notebooks/work/other.ipynb")
+ }
+ }
Review Comment:
[e0418f8](https://github.com/apache/texera/pull/7602/commits/e0418f844ea6d139dae7769a9610e4a92f292065)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3780828080
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -457,9 +463,15 @@ class NotebookMigrationResource extends LazyLogging {
@GET
@Path("/get-jupyter-iframe-url")
- def getJupyterIframeURL(@Auth user: SessionUser): Response = {
+ def getJupyterIframeURL(
+ @QueryParam("notebookName") notebookName: String,
Review Comment:
This is new user-facing API support: callers can now select a notebook via a query parameter. The repository's contribution guidance says support that did not previously exist is a `feat`, while `refactor` asserts identical user-facing behavior. Please rename the PR/commit to `feat(notebook-migration-service): compute jupyter iframe url per request`.
##########
notebook-migration-service/src/test/scala/org/apache/texera/service/resource/NotebookMigrationResourceSpec.scala:
##########
@@ -481,9 +481,42 @@ class NotebookMigrationResourceSpec
urlResp.getStatus shouldBe Response.Status.OK.getStatusCode
urlResp.getEntity.toString should include("localhost:9100")
- val iframeResp = resource.getJupyterIframeURL(sessionUser(writerUid))
+ val iframeResp = resource.getJupyterIframeURL(null, sessionUser(writerUid))
iframeResp.getStatus shouldBe Response.Status.OK.getStatusCode
- iframeResp.getEntity.toString should include("/notebooks/work/")
+ iframeResp.getEntity.toString should include("/notebooks/work/notebook.ipynb")
+ }
+ }
+
+ it should "build the iframe URL from an explicit notebook name" in {
+ withFakeJupyter(contentsStatus = 201) {
+ val resp = NotebookMigrationResource.getJupyterIframeURL("other.ipynb")
+ resp.getStatus shouldBe Response.Status.OK.getStatusCode
+ resp.getEntity.toString should include("/notebooks/work/other.ipynb")
+ }
+ }
Review Comment:
This positive test calls the companion helper directly, so it would still pass if the JAX-RS wrapper ignored every explicit `notebookName` and always used the default. Exercise `resource.getJupyterIframeURL` here so the newly added endpoint forwarding is covered.
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -75,13 +75,10 @@ object NotebookMigrationResource extends LazyLogging {
private val jupyterUrl = StorageConfig.jupyterURL
private val jupyterToken = StorageConfig.jupyterToken
- // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
- // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
- // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
- // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
- // per-user keying here, or one user's upload would overwrite another's iframe URL.
- @volatile private var jupyterIframeURL =
- s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ // Default notebook name used when a request does not specify one, so a param-less
+ // getJupyterIframeURL call reproduces the URL from before this service became stateless.
+ private val defaultNotebookName = "notebook.ipynb"
Review Comment:
Removing the process-global iframe state leaves `frontend/src/app/workspace/service/notebook-migration/notebook-migration.service.ts:130-133` factually stale: it still says a global service needs keying for the backend's `process-global jupyterIframeURL`. Update that comment as part of this refactor so future work does not rely on state that no longer exists.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on PR #7602:
URL: https://github.com/apache/texera/pull/7602#issuecomment-5284400625
/request-review @mengw15
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3781297619
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -75,13 +75,10 @@ object NotebookMigrationResource extends LazyLogging {
private val jupyterUrl = StorageConfig.jupyterURL
private val jupyterToken = StorageConfig.jupyterToken
- // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
- // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
- // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
- // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
- // per-user keying here, or one user's upload would overwrite another's iframe URL.
- @volatile private var jupyterIframeURL =
- s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ // Default notebook name used when a request does not specify one, so a param-less
+ // getJupyterIframeURL call reproduces the URL from before this service became stateless.
+ private val defaultNotebookName = "notebook.ipynb"
Review Comment:
Following up: #7390 notes the shared state "blocks running the service as a single global instance", and it closes with this PR — so with the warning comment going too, I'm not sure what's left tracking the jupyterUrl/jupyterToken half. Maybe a follow-up issue like #7636?
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3786608743
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -104,8 +101,17 @@ object NotebookMigrationResource extends LazyLogging {
}
}
- // Returns the Jupyter iframe reference URL
- def getJupyterIframeURL(): Response = {
+ // Returns the Jupyter iframe reference URL for the given notebook.
+ def getJupyterIframeURL(notebookName: String): Response = {
+ // notebookName flows into the returned URL, so validate it the same way setNotebook does:
Review Comment:
Fixed in [f86a023](https://github.com/apache/texera/pull/7602/commits/f86a02379f633f17f2ab3c08031c785bc9bc939f)
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3786613573
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -75,13 +75,10 @@ object NotebookMigrationResource extends LazyLogging {
private val jupyterUrl = StorageConfig.jupyterURL
private val jupyterToken = StorageConfig.jupyterToken
- // The token is passed as a URL param so the browser iframe can authenticate when loading the notebook.
- // jupyterIframeURL is process-global state. This is safe ONLY because each user runs their own pod
- // (own notebook-migration-service JVM + own Jupyter) in the k8s deployment, so this singleton is
- // effectively per-user. Do NOT deploy this service as a shared multi-user instance without adding
- // per-user keying here, or one user's upload would overwrite another's iframe URL.
- @volatile private var jupyterIframeURL =
- s"$jupyterUrl/notebooks/work/notebook.ipynb?token=$jupyterToken"
+
+ // Default notebook name used when a request does not specify one, so a param-less
+ // getJupyterIframeURL call reproduces the URL from before this service became stateless.
+ private val defaultNotebookName = "notebook.ipynb"
Review Comment:
Added a comment in [bf9035e](https://github.com/apache/texera/pull/7602/commits/bf9035e3836b13cd6cf645a1503d23564770bc37) and opened #7665 to track this
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on code in PR #7602:
URL: https://github.com/apache/texera/pull/7602#discussion_r3786463994
##########
notebook-migration-service/src/main/scala/org/apache/texera/service/resource/NotebookMigrationResource.scala:
##########
@@ -457,9 +463,15 @@ class NotebookMigrationResource extends LazyLogging {
@GET
@Path("/get-jupyter-iframe-url")
- def getJupyterIframeURL(@Auth user: SessionUser): Response = {
+ def getJupyterIframeURL(
+ @QueryParam("notebookName") notebookName: String,
Review Comment:
Fixed
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 merged PR #7602:
URL: https://github.com/apache/texera/pull/7602
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "zyratlo (via GitHub)" <gi...@apache.org>.
zyratlo commented on PR #7602:
URL: https://github.com/apache/texera/pull/7602#issuecomment-5299186670
> LGTM. The added comment plus #7665 covers what I was worried about — the constraint now has a home in both the code and the tracker. Deferring the frontend filename change until #7601 lands makes sense; worth a sub-issue for it, since this PR closes and the note goes with it.
I have raised issue #7671 to keep track of the frontend change
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor(notebook-migration-service): compute jupyter iframe url per request [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7602:
URL: https://github.com/apache/texera/pull/7602#issuecomment-5273630424
<!-- texera-reviewer-suggestion -->
### 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.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org