You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The Docker launcher's isAlive() isn't really implemented. It just calls isRunning(), which only checks whether the interpreter's Thrift port accepts a connection (checkIfRemoteEndpointAccessible). That tells you the port is open, not that the process behind it is alive. A container can be OOMKilled or already gone and still look alive if the port happens to answer for a moment. It also doesn't match the InterpreterClient contract, which says isAlive should reflect process status and stay separate from running. The method still had //TODO(ZEPPELIN-5876): Implement it more accurately sitting on it.
While I was there I filled in getErrorMessage(), which always returned null. When the container isn't running it now says why: OOMKilled, or a non-zero exit code. That logic lives in a small describeContainerFailure() helper to keep getErrorMessage() short.
One change is test-only: the docker field is now 'VisibleForTesting', so tests can inject a mock DockerClient and skip needing a real daemon.
What type of PR is it?
Feature
Todos
Implement isAlive() from the container's actual state (running/paused)
1. (should address) An unrelated existing test was removed
start_removesContainer_evenWhenKillFailsDuringCleanup was deleted in this PR. That test covered a distinct rollback-resilience case (removeContainer must still fire even when both startContainer and killContainer throw during cleanup), which is unrelated to the isAlive / getErrorMessage change here. The remaining start_removesContainer_whenContainerPreparationFails only exercises the kill-succeeds path, so this scenario is no longer covered. Looks like it may have been dropped accidentally while adding the new tests. Could you restore it?
2. (minor) Test name is broader than what it verifies
isAlive_falseWhenContainerExitedOrOomKilled only sets running=false and never touches oomKilled. Since isAlive only looks at running/paused and does not distinguish exited vs OOMKilled, the behavior is fine, but the name promises more coverage than exists. Renaming to something like isAlive_falseWhenContainerNotRunning would match the actual assertion. The OOMKilled path is already covered by getErrorMessage_reportsOomKilled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this PR for?
The Docker launcher's isAlive() isn't really implemented. It just calls isRunning(), which only checks whether the interpreter's Thrift port accepts a connection (checkIfRemoteEndpointAccessible). That tells you the port is open, not that the process behind it is alive. A container can be OOMKilled or already gone and still look alive if the port happens to answer for a moment. It also doesn't match the InterpreterClient contract, which says isAlive should reflect process status and stay separate from running. The method still had //TODO(ZEPPELIN-5876): Implement it more accurately sitting on it.
While I was there I filled in getErrorMessage(), which always returned null. When the container isn't running it now says why: OOMKilled, or a non-zero exit code. That logic lives in a small describeContainerFailure() helper to keep getErrorMessage() short.
One change is test-only: the docker field is now 'VisibleForTesting', so tests can inject a mock DockerClient and skip needing a real daemon.
What type of PR is it?
Feature
Todos
What is the Jira issue?
[ZEPPELIN-5876]
How should this be tested?
Automated unit tests in DockerInterpreterProcessTest :
Screenshots (if appropriate)
Questions: