Repository navigation
[ZEPPELIN-6737] Connect a private read-only Angular notebook view to Shared Notebook Core - #5568
Conversation
GET_NOTE and NOTE_REVISION replies, their AUTH_INFO and ERROR_INFO failures, and errors from the top-level handler for these two requests now carry the msgId the client sent, so a client can tell its own read reply from a NOTE broadcast or a reply to an earlier request. Broadcasts, other operations and requests without a msgId keep the previous payloads.
…vation The SDK read requests return the msgId they sent. NotebookReadTransport listens before it sends and passes a Core read request only the first NOTE, NOTE_REVISION, AUTH_INFO or ERROR_INFO carrying its msgId, then stops listening. Broadcasts, replies to other requests and replies after a timeout or cancellation never reach the Core. The permissions read keeps the ACL handle of the activation it started in, so both its success and its failure are rejected once the route has moved on. A 403 is reported as access-denied and anything else as failed. A missing note is only described by ERROR_INFO text, so it is reported as failed rather than guessed from the message.
NotebookCoreReadHost creates one read store and one frozen NotebookPort, keeps them across note and revision changes, and exposes the snapshot with the Shell's connection state as one replayed Observable. It sends a read only while the Shell's socket is connected. When the connection drops it ends pending reads and replaces any loaded note with a failed state, so stale ready state does not survive. It reads the current target again when the Shell reconnects, without a reconnect loop of its own. Disposing ends every read and the Core, and a second dispose changes nothing.
With `?notebookCoreRead` on a notebook URL, a canMatch guard on the existing route boundary selects NotebookCoreReadComponent instead of the editable notebook. Without it the route is unchanged, and leaving the flag destroys the private view and loads the editable notebook afresh. The view renders loaded snapshots and saved results only. It does not mount the editable paragraph components, so it sends no run, save, patch, form or configuration message, including on mount and unmount. Saved results go through the shared result component in published mode, with a per-render copy of the persisted chart settings because the component writes into its config. ANGULAR and NETWORK output and unknown result types are shown as not rendered instead of being skipped. Permissions are shown for a loaded note only and stay unknown when their read fails.
The spec runs the private view against a real server, through a Playwright WebSocket route that records what the page sends and can hold, inject or drop server frames. It checks the rendered saved results and chart settings, that mounting and unmounting send only the read, A -> B -> A with a late reply, a pending read that a NOTE broadcast cannot complete, duplicate replies and broadcasts after loading, revision entry, exit and re-entry, cancelled and failed permission reads, a missing note, invalidation on a dropped connection with a fresh read after the Shell reconnects, and, with two Shiro accounts, a read the note permissions deny.
Two live captures record what the private view reads: a live note, a revision and a missing note in anonymous mode, and a read the note permissions deny in authenticated mode. Each read reply and each failure carries the msgId of its request. The permissions reads answer 200 for all three anonymous reads, the missing note included, and 403 for the denied read. The replies depend on the server change in this branch, which is not on Apache master yet, so the captures name the commit that was built and served and are kept apart from the lifecycle capture inventory. The README section says to recapture from master once the change is merged. AUTH_INFO prints account names in its text, which the capture replaces with <user>. A replay spec drives the production read transport and Core with both captures in recorded order and fails if the replies lose their msgId.
voidmatcha
left a comment
voidmatcha
left a comment
There was a problem hiding this comment.
Thanks for the PR. I built it and ran it locally, and it works as described.
What I checked
- Links I used: the read view at
http://localhost:<port>/#/notebook/<noteId>?notebookCoreRead, a revision at.../#/notebook/<noteId>/revision/<revisionId>?notebookCoreRead, and a missing note or revision with a non-existent id in the same form. For comparison I opened the normal notebook without the flag at.../#/notebook/<noteId>. - Browser: notes and revisions load, saved results (table, HTML) render, and ANGULAR/NETWORK show as "not shown". A missing note or revision shows a failure with Retry instead of a blank screen. Removing the flag gives the normal notebook, adding it back gives the read view, and the read view sends only
GET_NOTE/NOTE_REVISION. - Server:
NotebookServerTestpasses 36/36 on a clean clone. Probing the WebSocket directly, themsgIdcomes back correctly onNOTE, onERROR_INFOfor a missing note, on the top-level exception path, and for overlapping reads, while requests without amsgId, unrelated operations and broadcasts behave as before. - Playwright
core-read: anonymous mode 7 passed (the denied-read test is skipped without two accounts); Shiro mode passed all 8, including access denied. On local Git storage the revision test needed one retry because parallel tests race on the JGit index lock.
I put a few small, optional suggestions on a branch you can cherry-pick from: https://github.com/voidmatcha/zeppelin/tree/ZEPPELIN-6737-pr5568-review-fixes (details inline).
Note: I understand this read view as the "private comparison harness" from ZEPPELIN-6627, so I expect core-read/, this guard and their e2e specs to be retired together at some point (timing TBD). So my suggestions focus on what will last (the msgId correlation and the fixtures); on the view side I only put small things on the branch (the label and a few role attributes). Since that code will go away, feel free to skip it.
| @if (view.snapshot.status === 'ready') { | ||
| @if (view.snapshot.acl.status === 'ready') { | ||
| <span class="core-read-permissions" data-testid="notebook-core-read-permissions"> | ||
| Your access: {{ view.access }} |
There was a problem hiding this comment.
The note lists do not know the server's admin override, so a loaded note can show "none" here. The branch hides the label in that case; removing it is fine too.
There was a problem hiding this comment.
Thanks, cherry-picked as b007a4c (with the role attributes). A loaded note now shows no access label when the lists do not name the user.
| }, | ||
| capturedAt: new Date().toISOString(), | ||
| zeppelinVersion: version.version, | ||
| source: { repository: 'kimyenac/zeppelin', branch: 'ZEPPELIN-6737', commit } |
There was a problem hiding this comment.
The replay spec uses the recorded replies as-is; only this source label is never read, so leaving it pointing at the fork commit is fine for this PR. Recapturing from master after the merge fixes it, and I think you could open one more issue related to ZEPPELIN-6673 or send a [MINOR] PR. The branch's capture spec records apache/zeppelin and the served serverCommit instead of hard-coding the fork.
There was a problem hiding this comment.
Cherry-picked as 4c21fa4. In d7ea6ef I also dropped the "until then the source is the named branch commit" exclusion and reworded the README, since the capture spec now records apache/zeppelin. The committed captures still name the branch commit. I will recapture them from Apache master as part of ZEPPELIN-6673 after this is merged, rather than opening a new issue.
| noteId: recorded.data.id ?? recorded.data.noteId, | ||
| revisionId: recorded.data.revisionId | ||
| }); | ||
| return recorded.msgId as string; |
There was a problem hiding this comment.
If the recorded msgId were lost from both the sent read and its reply, undefined would match undefined, so the replay would still pass. The branch asserts that the recorded request has a non-empty msgId.
There was a problem hiding this comment.
Good catch. Cherry-picked as 4c21fa4, so the replay now fails when a recorded read has no msgId.
|
Merged into master (e8d710f). |
2 participants
What is this PR for?
Connect the Shared Notebook Core read state from ZEPPELIN-6736 to a private, read-only Angular notebook view that reads real notebook and revision data. The view is opt-in for development and testing with
?notebookCoreReadon a notebook URL (#/notebook/<id>?notebookCoreRead, also under/revision/<revisionId>). Without the flag the editable notebook route is unchanged.Commits:
GET_NOTEandNOTE_REVISIONreplies, theirAUTH_INFOandERROR_INFOfailures, and errors from the top-levelonMessagehandler for these two requests carry the requestmsgId. Broadcasts, other operations and requests without amsgIdkeep their previous payloads.getNoteandnoteRevisionreturn themsgIdthey sent.NotebookReadTransportsubscribes before it sends, accepts only the first reply carrying itsmsgId, then stops listening. It bounds each read with a timeout and keeps the Core's ACL handle for the permissions REST read, so an old success or failure cannot apply.NotebookCoreReadHostowns one read store and one frozenNotebookPortfor one private route lifetime. It reads only while the Shell's socket is connected, replaces loaded state withfailedwhen the connection drops, reads again after the Shell reconnects without a reconnect loop of its own, and disposes once.canMatchguard on the existing route boundary (ZEPPELIN-6675) selectsNotebookCoreReadComponent. The component renders snapshots through a reused Observable (asyncpipe, OnPush) and does not mount the editable paragraph components.Read operations
GET_NOTE→NOTEwith the requestmsgIdNOTE_REVISION→NOTE_REVISIONwith the requestmsgId. ANOTEbroadcast does not change a loaded revisionGET /api/notebook/{noteId}/permissions;403isaccess-denied, other failuresfailed, and permissions stay unknown on failureAUTH_INFOwith the requestmsgId→access-deniedERROR_INFOwith the requestmsgId, an empty reply, or a timeout →failedA missing note is reported as
failed. The server only describes it inERROR_INFOtext, which this PR does not parse. The server also answers the permissions read of a missing note with200, so the view shows permissions only for a loaded note.Excluded: run, cancel, save, commit, patch, form and configuration changes, incremental paragraph and output updates, collaboration, Angular objects, ACL writes and reconnect recovery beyond reading again. The view sends no message other than its reads, including while it mounts and unmounts.
Saved results rendered:
TABLE(with persisted chart settings),TEXT,HTML,IMG,SVG,NULL. Shown as not rendered:ANGULAR(needs live Angular objects),NETWORK(no Angular renderer) and unknown types. The result component writes defaults into its config while it renders (renderGraph→setDefaultConfig), so it receives a per-render copy of the frozen chart settings that is never sent back.Fixture provenance
e2e/fixtures/notebook-read/read-anonymous.jsonandread-denied.jsonwere captured from this branch (server and UI built from the PR commits), because the replies depend on the server change above, which is not on Apache master yet. They are kept apart from the seven lifecycle captures and their inventory, andmetadata.sourcenames the branch commit. The core-contract README says to recapture them from master once this is merged.AUTH_INFOprints account names in its text, so the capture replaces them with<user>. The ACL and error evidence here covers the reads of this view only; the rest of ZEPPELIN-6673 (ACL writes, REST 401/405,SESSION_LOGOUT) is left to that issue.What type of PR is it?
Feature
Todos
e2e/fixtures/notebook-read/from Apache master after this is mergedWhat is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6737
How should this be tested?
Server (
NotebookServerTest, 36 tests; 6 new):./mvnw test -pl zeppelin-server -Dtest=NotebookServerTestThe new tests cover the
msgIdonNOTE,NOTE_REVISION,AUTH_INFOandERROR_INFOread replies, on the top-level exception path for both reads, and its absence on requests without amsgId, on an unrelated operation's failure and top-level error, and onNOTEbroadcasts. Removing each server change made the corresponding test fail.Frontend, from
zeppelin-web-angular:The core-read unit tests cover the transport (listen-before-send,
msgIdmatching, duplicate, late, post-timeout and post-cancel replies, A → B → A, permission success and failure for an earlier activation), the host (port identity, connection loss and reconnect, exactly-once teardown), the view model and the component (only reads are sent on mount, route changes and unmount). Breaking each of these behaviors made a test fail. The replay spec runs both captures through the production transport and Core, and fails if the captured replies lose theirmsgId.Real-route Playwright (
e2e/tests/notebook/core-read/notebook-core-read.spec.ts), Chromium, against a server built from this branch:conf/shiro.ini.templatewithadminadded touser1, as in CI): 8 passedA Playwright WebSocket route records every frame the page sends and holds, injects or drops server frames. Removing the
msgIdcheck made the pending-read broadcast test fail, and keeping loaded state on a dropped connection made the reconnect test fail.Default notebook regression with the flag off (authenticated,
tests/notebook/{main,paragraph,persistence,revision}): 25 passed, 6 failed for local environment reasons, not this change: 4 React footer tests (the React remote was not built for this local dist,remoteEntry.js404) and 2%pythonexecution tests (no working local Python interpreter). Full CI covers both legs.Not run locally: Firefox and WebKit, and the full E2E suite.
Captures were recorded with:
Screenshots (if appropriate)
Questions:
msgIdget the same payloads, and the SDK methods only gain a return value.🤖 Generated with Claude Code