-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6340] Add ZeppelinEventBus and update NotebookServer to handle NoteRemoveEvent #5085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
I've just merged the PRs that fixed the CI failures into master. |
|
I've done it. Thanks. |
|
@seung-00 I found your merge commit(daa45c1) is empty. Could you rebase again? Because CI Failure caused by zeppelin-zengine issue. It was solved(#5081) and merged to master branch. https://github.com/apache/zeppelin/actions/runs/18057550292/job/51390882307?pr=5085#step:10:16071 |
|
@dididy |
|
I didn’t carefully review the PR description, so I didn’t realize it also included changes related to zeppelin-zengine. It seems that some of the current test failures(NullPointer) are likely due to those modifications. https://github.com/apache/zeppelin/actions/runs/18057550292/job/51390882307?pr=5085#step:10:16003 CI Error detailError: Errors: Error: NotebookTest.testAbortParagraphStatusOnInterpreterRestart:1461 » NullPointer Error: NotebookTest.testAngularObjectRemovalOnInterpreterRestart:1262 » NullPointer Error: NotebookTest.testAngularObjectRemovalOnNotebookRemove:1179 » NullPointer Error: NotebookTest.testAngularObjectRemovalOnParagraphRemove:1229 » NullPointer Error: NotebookTest.testClearParagraphOutput:474 » NullPointer Error: NotebookTest.testCloneNote:1116 » NullPointer Error: NotebookTest.testCreateDuplicateNote:1737 » NullPointer Error: NotebookTest.testCreateNoteWithSubject:450 » NullPointer Error: NotebookTest.testCronNoteInTrash:1019 » NullPointer Error: NotebookTest.testExportAndImportNote:1068 » NullPointer Error: NotebookTest.testGetAllNotes:1724 » NullPointer Error: NotebookTest.testInvalidInterpreter:578 » NullPointer Error: NotebookTest.testMoveNote:1898 » NullPointer Error: NotebookTest.testMoveNote:1898 » NullPointer Error: NotebookTest.testMoveNote:1898 » NullPointer Error: NotebookTest.testPerNoteSessionInterpreter:1569->lambda$testPerNoteSessionInterpreter$50:1631 » NullPointer Error: NotebookTest.testPerSessionInterpreter:1512->lambda$testPerSessionInterpreter$48:1556 » NullPointer Error: NotebookTest.testPerSessionInterpreterCloseOnNoteRemoval:1491 » NullPointer Error: NotebookTest.testPermissions:1315 » NullPointer Error: NotebookTest.testPersist:438 » NullPointer Error: NotebookTest.testRemoveCorruptedNote:551 » NullPointer Error: NotebookTest.testResourceRemovealOnParagraphNoteRemove:1151 » NullPointer Error: NotebookTest.testRunAll:616 » NullPointer Error: NotebookTest.testRunBlankParagraph:496 » NullPointer Error: NotebookTest.testSchedule:660 » NullPointer Error: NotebookTest.testScheduleAgainstRunningAndPendingParagraph:714 » NullPointer Error: NotebookTest.testScheduleDisabled:776->terminateScheduledNote:834 » NullPointer Error: NotebookTest.testScheduleDisabledWithName:803->terminateScheduledNote:834 » NullPointer Error: NotebookTest.testSchedulePoolUsage:737->terminateScheduledNote:834 » NullPointer Error: NotebookTest.testSelectingReplImplementation:321 » NullPointer [INFO] Error: Tests run: 266, Failures: 0, Errors: 30, Skipped: 6 |
fb157db to
605499c
Compare
- Add ZeppelinEventBus - Update NotebookServer to handle NoteRemoveEvent using EventBus
|
I am currently against the introduction of an event bus. As Jongyoul Lee wrote in an email, we should use the existing resources to ensure communication. https://lists.apache.org/thread/nk85rcn4qomxgpw56xjd804h3j9w7vz7 It has been on the roadmap for some time to combine the Zeppelin server and zeppelin-zengine code into one module, as the separation has proven to be a mistake. Currently, the providers are only available in zeppelin-server. This also became apparent to me when I tried to improve the plugin integration. See #4355 (comment) |
|
To add some clarity to my previous comment: I strongly support the goal of this PR to remove the circular dependency. However, I agree with @Reamer that adding an event bus to zeppelin-zengine (which is only used by zeppelin-server) isn't the right solution. This is a symptom of a larger problem: the fact that these two modules are separate to begin with. My suggestion is to first merge zeppelin-server and zeppelin-zengine. Once that is done, we can re-apply the dependency removal logic from this PR. This approach fixes the root architectural issue and allows this PR's contribution to be implemented cleanly. I'm open to discussing this plan. |
What is this PR for?
This PR introduces the initial step of refactoring Zeppelin’s listener-based architecture to an EventBus model(proposal).
It establishes the necessary infrastructure and refactors the
NoteRemoveEventflow as the initial target.Key Changes:
ZeppelinEventBusclasszeppelin-zenginemodulezeppelin.eventbus.enabledfeature flagNotebookServer(implements NoteEventListener) to handleNoteRemoveEventviaZeppelinEventBuswhenzeppelin.eventbus.enabledis enabledWhat type of PR is it?
Improvement
Todos
What is the Jira issue?
How should this be tested?
Screenshots (if appropriate)
Questions: