Skip to content

[ZEPPELIN-6216] Show clear message when job manager is disabled instead of infinite loading#5035

Merged
tbonelee merged 2 commits into
apache:masterfrom
devyeony:feature/ZEPPELIN-6216
Aug 28, 2025
Merged

[ZEPPELIN-6216] Show clear message when job manager is disabled instead of infinite loading#5035
tbonelee merged 2 commits into
apache:masterfrom
devyeony:feature/ZEPPELIN-6216

Conversation

@devyeony

Copy link
Copy Markdown
Contributor

This PR is a rebased version of #5022
The previous PR was automatically closed because the branch was deleted, so I recreated the branch and opened this new PR.

What is this PR for?

This PR fixes the issue that occurs when zeppelin.jobmanager.enable is set to false. In this case, the server sends no response, causing the Job Manager page in both classic and new UIs to show an infinite loading indicator. This behavior confuses users and gives the impression that the page is broken.

To improve user experience, this PR introduces explicit server-side handling that returns an explicit "not allowed" response when the Job Manager is disabled via configuration. Additionally, both classic and new UIs have been updated to properly handle this response and display a user-friendly message: "Job Manager is disabled in the current configuration."

This change ensures users are informed about the disabled state instead of encountering an endless loading screen.

What type of PR is it?

Improvement

Todos

  • Implement server-side forbidden response when Job Manager is disabled
  • Add JOB_MANAGER_DISABLED WebSocket message and corresponding handling in both Classic and New UI
  • Add unit and integration tests covering the new behavior

Note:

  • Although REST API tests have been implemented, the current frontend (both Classic and New UI) interacts with the Job Manager primarily through WebSocket communication. Therefore, the functional verification and actual handling of the "Job Manager disabled" scenario have been focused on the WebSocket path. The REST API tests are prepared to ensure future compatibility if the REST endpoints are used.
  • In the user-request handling method, a ForbiddenException triggers sending a clear error message to the client so the UI can inform the user that the Job Manager is disabled. However, during broadcast events where no direct client request is involved, the exception is logged at debug level and silently skipped to avoid unnecessary noise or client interruptions.

What is the Jira issue?

[ZEPPELIN-6216]

How should this be tested?

  • Automated tests have been added to cover the new behavior when the Job Manager is disabled:
    • Unit tests in JobManagerServiceTest verify that forbidden exceptions are thrown.
    • REST API tests in NotebookRestApiTest check that HTTP 403 responses with appropriate messages are returned.
    • WebSocket and server event handling tests in NotebookServerTest ensure no unexpected exceptions occur and proper notification messages are sent.
  • Manual testing steps:
    1. Set zeppelin.jobmanager.enable to false via ZeppelinConfiguration.
    2. Access the Job Manager page in both classic and new UI.
    3. Confirm that instead of infinite loading, a clear message stating "Job Manager is disabled in the current configuration." is displayed.

Screenshots (if appropriate)

  • AS-IS (New UI)
AS-IS NEW UI
  • AS-IS (Classic UI)
AS-IS CLASSIC UI
  • TO-BE (New UI)
TO-BE NEW UI
  • TO-BE (Classic UI)
TO-BE CLASSIC UI

Questions:

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

@devyeony

Copy link
Copy Markdown
Contributor Author

The original branch was deleted, so I recreated it and opened this new PR.
You can refer to the previous code review in #5022.
Sorry for the inconvenience.

I’ve rebased and merged up to the latest commits.

@Reamer
I’ve addressed all the points you mentioned in your review. I'd appreciate it if you could take a look when you have time. Thank you so much!

@tbonelee
I’ve addressed all the points you mentioned in your review, except for the issue regarding the ng-if="jobManagerEnabled" block inside the ng-if="isFilterLoaded" block. Could you take a look at that part when you have time? Thanks a lot!

@devyeony devyeony force-pushed the feature/ZEPPELIN-6216 branch from 46e70e5 to eeefcbf Compare August 20, 2025 15:48
Reamer
Reamer previously approved these changes Aug 21, 2025

@Reamer Reamer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backend Code LGTM
We need a review for the frontend part.

@Reamer Reamer self-assigned this Aug 21, 2025
@Reamer Reamer requested a review from tbonelee August 21, 2025 05:58
tbonelee
tbonelee previously approved these changes Aug 21, 2025

@tbonelee tbonelee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Frontend code LGTM.
I left a comment in the previous PR suggesting a change related to the ng-if block nesting.
It's a just code structure suggestion, not a logical issue, so feel free to share your thoughts on it.

ParkGyeongTae
ParkGyeongTae previously approved these changes Aug 23, 2025

@ParkGyeongTae ParkGyeongTae left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change will help reduce user confusion.

@devyeony

Copy link
Copy Markdown
Contributor Author

@Reamer @tbonelee @ParkGyeongTae Thank you for your review and feedback.
@tbonelee Thank you for your valuable feedback. I will take a closer look at the points you mentioned.

@ParkGyeongTae

Copy link
Copy Markdown
Member

@devyeony There are conflicts with master. Please rebase your branch on master and force-push to fix them.

@devyeony devyeony dismissed stale reviews from ParkGyeongTae, tbonelee, and Reamer via 095ab99 August 24, 2025 13:59
@devyeony devyeony force-pushed the feature/ZEPPELIN-6216 branch from eeefcbf to 095ab99 Compare August 24, 2025 13:59
@devyeony

Copy link
Copy Markdown
Contributor Author

@ParkGyeongTae Conflicts with master were resolved via rebase and force-push.
The conflicts in job-manager.component.html were caused by formatting changes.
I applied the lint and formatting updates from ZEPPELIN-6287.

ParkGyeongTae
ParkGyeongTae previously approved these changes Aug 24, 2025
@Reamer

Reamer commented Aug 25, 2025

Copy link
Copy Markdown
Contributor

We have a CI failure.

Error:  Failures: 
Error:    JobManagerServiceTest.shouldThrowForbiddenException_whenJobManagerIsDisabled:53 Expected org.apache.zeppelin.service.exception.JobManagerForbiddenException to be thrown, but nothing was thrown.

…disabled

Previously, JobManagerService threw JobManagerForbiddenException immediately when zeppelin.jobmanager.enable = false. After code review, the behavior
was changed to return Collections.emptyList() and report the exception via
the callback. This commit updates the tests to reflect the new behavior
and prevent CI failures.
@devyeony

devyeony commented Aug 25, 2025

Copy link
Copy Markdown
Contributor Author

@Reamer, thank you for your feedback on the parts I missed. I would appreciate it if you could take a look at this test code JobManagerServiceTest and share your thoughts when you have time.

⚠️ CI Failure Reason:
Initially, the code was designed to throw a JobManagerForbiddenException immediately when zeppelin.jobmanager.enable = false. After the code review, it was modified to return Collections.emptyList() and report the exception via the callback instead. The related test code was not updated to reflect this change, which caused the CI failure.

✨ Test Refactoring:
All tests for zeppelin.jobmanager.enable = false have been grouped under a @Nested class. This improves organization and readability compared to the previous version, making it easier to understand which tests correspond to the disabled JobManager scenario.

💡 Future Improvements:
While updating the test code, I identified a few areas for potential improvements. These are beyond the scope of this PR and were not included, but they could be addressed in follow-up work.

  1. This PR focuses on the case when zeppelin.jobmanager.enable = false. Test cases for zeppelin.jobmanager.enable = true were not included in this PR and could be added in a future PR.
  2. Currently, MockitoExtension is not applied in the project. As a result, we use @SuppressWarnings("unchecked") when creating generic mocks, which appears in other tests as well. In the future, we could refactor tests to use MockitoExtension and @Mock annotations for a more modern and type-safe style.

@Reamer Reamer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tbonelee tbonelee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ParkGyeongTae ParkGyeongTae left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍
Just to note, the CI failure in license-check (pull_request) is already fixed in another PR, so no problem.
See: #5047

@tbonelee tbonelee merged commit fbd94da into apache:master Aug 28, 2025
15 of 18 checks passed
tbonelee pushed a commit that referenced this pull request Aug 28, 2025
…ad of infinite loading

> This PR is a rebased version of [#5022](#5022)
> The previous PR was automatically closed because the branch was deleted, so I recreated the branch and opened this new PR.

### What is this PR for?
This PR fixes the issue that occurs when `zeppelin.jobmanager.enable` is set to `false`. In this case, the server sends no response, causing the Job Manager page in both classic and new UIs to show an infinite loading indicator. This behavior confuses users and gives the impression that the page is broken.

To improve user experience, this PR introduces explicit server-side handling that returns an explicit "not allowed" response when the Job Manager is disabled via configuration. Additionally, both classic and new UIs have been updated to properly handle this response and display a user-friendly message: **"Job Manager is disabled in the current configuration."**

This change ensures users are informed about the disabled state instead of encountering an endless loading screen.

### What type of PR is it?
Improvement

### Todos
* [x] Implement server-side forbidden response when Job Manager is disabled
* [x] Add `JOB_MANAGER_DISABLED` WebSocket message and corresponding handling in both Classic and New UI
* [x] Add unit and integration tests covering the new behavior

**Note:**
* Although REST API tests have been implemented, the current frontend (both Classic and New UI) interacts with the Job Manager primarily through WebSocket communication. Therefore, the functional verification and actual handling of the "Job Manager disabled" scenario have been focused on the WebSocket path. The REST API tests are prepared to ensure future compatibility if the REST endpoints are used.
* In the user-request handling method, a ForbiddenException triggers sending a clear error message to the client so the UI can inform the user that the Job Manager is disabled. However, during broadcast events where no direct client request is involved, the exception is logged at debug level and silently skipped to avoid unnecessary noise or client interruptions.

### What is the Jira issue?
[[ZEPPELIN-6216]](https://issues.apache.org/jira/browse/ZEPPELIN-6216)

### How should this be tested?
* Automated tests have been added to cover the new behavior when the Job Manager is disabled:
  * Unit tests in `JobManagerServiceTest` verify that forbidden exceptions are thrown.
  * REST API tests in `NotebookRestApiTest` check that HTTP 403 responses with appropriate messages are returned.
  * WebSocket and server event handling tests in `NotebookServerTest` ensure no unexpected exceptions occur and proper notification messages are sent.
* Manual testing steps:
  1. Set `zeppelin.jobmanager.enable` to `false` via `ZeppelinConfiguration`.
  2. Access the Job Manager page in both classic and new UI.
  3. Confirm that instead of infinite loading, a clear message stating **"Job Manager is disabled in the current configuration."** is displayed.

### Screenshots (if appropriate)
* AS-IS (New UI)
<img width="862" height="362" alt="AS-IS NEW UI" src="https://github.com/user-attachments/assets/e2a1288f-5952-4ea0-a31b-a267f49f171e" />

* AS-IS (Classic UI)
<img width="864" height="424" alt="AS-IS CLASSIC UI" src="https://github.com/user-attachments/assets/bc111aff-e95c-4427-a266-af7f9f600407" />

* TO-BE (New UI)
<img width="870" height="305" alt="TO-BE NEW UI" src="https://github.com/user-attachments/assets/0555e5dc-2ee8-4000-b1eb-c7d2259e96b1" />

* TO-BE (Classic UI)
<img width="875" height="275" alt="TO-BE CLASSIC UI" src="https://github.com/user-attachments/assets/e0a87e98-fde8-467b-9df3-d9ada39deb36" />

### Questions:
* Does the license files need to update? No
* Is there breaking changes for older versions? No
* Does this needs documentation? No

Closes #5035 from devyeony/feature/ZEPPELIN-6216.

Signed-off-by: ChanHo Lee <chanholee@apache.org>
(cherry picked from commit fbd94da)
Signed-off-by: ChanHo Lee <chanholee@apache.org>
@tbonelee

Copy link
Copy Markdown
Contributor

Merged into master and branch-0.12

@devyeony devyeony deleted the feature/ZEPPELIN-6216 branch August 29, 2025 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants