zeppelin icon indicating copy to clipboard operation
zeppelin copied to clipboard

[ZEPPELIN-6340] Add ZeppelinEventBus and update NotebookServer to handle NoteRemoveEvent

Open seung-00 opened this issue 3 months ago • 7 comments

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 NoteRemoveEvent flow as the initial target. Key Changes:

  • Added ZeppelinEventBus class
  • Added RxJava3 dependency in the zeppelin-zengine module
  • Added zeppelin.eventbus.enabled feature flag
  • Updated NotebookServer(implements NoteEventListener) to handle NoteRemoveEvent via ZeppelinEventBus when zeppelin.eventbus.enabled is enabled

What type of PR is it?

Improvement

Todos

  • [ ] Refactor other NoteEventListener methods and additional listeners to EventBus

What is the Jira issue?

  • Open an issue on Jira https://issues.apache.org/jira/browse/ZEPPELIN-6340

How should this be tested?

  • Run the existing test suite against the updated code
  • Verify the note deletion flow through the UI

Screenshots (if appropriate)

Questions:

  • Does the license files need to update? N
  • Is there breaking changes for older versions? N
  • Does this needs documentation? N

seung-00 avatar Sep 26 '25 18:09 seung-00

I've just merged the PRs that fixed the CI failures into master. Your PR doesn't have conflicts with master, but to ensure the CI runs cleanly, could you please rebase it onto master once?

tbonelee avatar Sep 27 '25 05:09 tbonelee

I've done it. Thanks. @tbonelee

seung-00 avatar Sep 27 '25 08:09 seung-00

@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 avatar Sep 27 '25 10:09 dididy

@dididy The changes in this PR branch don’t conflict with master, so the empty merge commit seems natural. The CI(core.yaml) failure is probably caused by the changes in this PR. I’ll check it. Thanks.

seung-00 avatar Sep 27 '25 10:09 seung-00

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/seung-00/zeppelin/blob/WIP/zeppelin-zengine/src/test/java/org/apache/zeppelin/notebook/NotebookTest.java

https://github.com/apache/zeppelin/actions/runs/18057550292/job/51390882307?pr=5085#step:10:16003

CI Error detail Error: 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

dididy avatar Sep 27 '25 10:09 dididy

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 https://github.com/apache/zeppelin/pull/4355#issuecomment-1234466448

Reamer avatar Sep 29 '25 06:09 Reamer

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.

jongyoul avatar Oct 04 '25 01:10 jongyoul