Skip to content

Conversation

@proceane
Copy link
Contributor

@proceane proceane commented Jul 13, 2025

What is this PR for?

In the zeppelin-server/rest module, it was observed that when running multiple tests together, configuration values of notebooks used in previous tests remain.
To address this, code has been added to shutdown serviceLocator when the test finishes and the server is stopped.

What type of PR is it?

Bug Fix

Todos

  • Add serviceLocatior shutdown

What is the Jira issue?

https://issues.apache.org/jira/browse/ZEPPELIN-6237

How should this be tested?

  • In the zeppelin-server/rest module, run test.

Screenshots (if appropriate)

Questions:

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

@proceane proceane force-pushed the feature/ZEPPELIN-6237 branch from 941e8a3 to 797adb9 Compare July 14, 2025 03:41
@Reamer
Copy link
Contributor

Reamer commented Jul 14, 2025

This pull request is a workaround for me, because the ServiceLocator should actually be executed when Zeppelin is shut down.

public void shutdown(int exitCode) {
if (!duringShutdown.getAndSet(true)) {
LOGGER.info("Shutting down Zeppelin Server ... - ExitCode {}", exitCode);
try {
if (jettyWebServer != null) {
jettyWebServer.stop();
}
if (sharedServiceLocator != null) {
if (!zConf.isRecoveryEnabled()) {
sharedServiceLocator.getService(InterpreterSettingManager.class).close();
}
sharedServiceLocator.getService(Notebook.class).close();
}
} catch (Exception e) {
LOGGER.error("Error while stopping servlet container", e);
}
LOGGER.info("Bye");
if (exitCode != 0) {
System.exit(exitCode);
}
}

Somewhere here should be sharedServiceLocator.shutdown();.

Unfortunately, my efforts failed because of the recovery tests.
Would you like to try a real shutdown here?

In my opinion, the close() method of the “services” must take the recovery mechanism into account and not at the top of the Zeppelin server.

if (sharedServiceLocator != null) {
if (!zConf.isRecoveryEnabled()) {
sharedServiceLocator.getService(InterpreterSettingManager.class).close();
sharedServiceLocator.shutdown();
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is the simple variant, but it actually belongs outside the ‘if’ statement.

ParkGyeongTae pushed a commit that referenced this pull request Sep 6, 2025
### What is this PR for?
This PR was created to prevent an error that occurs in the `InterpreterSetting.getInterpreterGroup` method when `groupId` is null and is retrieved from the Set.
Normally, the groupId parameter does not seem to be passed as null.
I found it when tested service shutdown(#4965).

### What type of PR is it?
Improvement

### Todos
* [x] - Add null check condition

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6282

### How should this be tested?

### Screenshots (if appropriate)

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


Closes #5026 from proceane/feature/ZEPPELIN-6282.

Signed-off-by: ParkGyeongTae <[email protected]>
ParkGyeongTae pushed a commit that referenced this pull request Sep 6, 2025
### What is this PR for?
This PR was created to prevent an error that occurs in the `InterpreterSetting.getInterpreterGroup` method when `groupId` is null and is retrieved from the Set.
Normally, the groupId parameter does not seem to be passed as null.
I found it when tested service shutdown(#4965).

### What type of PR is it?
Improvement

### Todos
* [x] - Add null check condition

### What is the Jira issue?
https://issues.apache.org/jira/browse/ZEPPELIN-6282

### How should this be tested?

### Screenshots (if appropriate)

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

Closes #5026 from proceane/feature/ZEPPELIN-6282.

Signed-off-by: ParkGyeongTae <[email protected]>
(cherry picked from commit 078b754)
Signed-off-by: ParkGyeongTae <[email protected]>
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.

2 participants