[FLINK-31931] Return 404 for a gone TaskManager and stop the Web UI from re-polling it#28735
Open
MartijnVisser wants to merge 2 commits into
Open
[FLINK-31931] Return 404 for a gone TaskManager and stop the Web UI from re-polling it#28735MartijnVisser wants to merge 2 commits into
MartijnVisser wants to merge 2 commits into
Conversation
…kManager in TaskManagerDetailsHandler TaskManagerDetailsHandler stripped the failure from its exceptionally stage with stripExecutionException, which does not unwrap the CompletionException that a CompletableFuture reports failures with. The instanceof UnknownTaskExecutorException check therefore never matched, so a request for a gone TaskManager returned 500 with an "Unhandled exception" ERROR log instead of the intended 404. Use stripCompletionException, as the sibling TaskManager handlers already do. Generated-by: Claude Opus 4.8 (1M context)
The TaskManager status and metrics views re-requested the TaskManager detail on every refresh tick, so a TaskManager that had been lost kept generating failing /taskmanagers/<id> requests indefinitely. loadManager also swallowed every error, hiding the not-found case from callers. Let loadManager propagate errors and share a small poll helper that stops once the backend reports the TaskManager is gone (404), while transient errors keep retrying. The status view then shows a "no longer available" notice instead of an endless skeleton, and the metrics cards are hidden instead of rendering blank tables. Generated-by: Claude Opus 4.8 (1M context)
Collaborator
Contributor
|
@MartijnVisser Can you add before and after images to the PR. |
|
Is "410 Gone" a better response code here if we knew the TaskManager was there but is gone and this is permanent? https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Status/410 Otherwise, 404 default should be fine. |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 the purpose of the change
Requesting
/taskmanagers/:taskmanageridfor a TaskManager that is no longer registered (for example after a TaskManager is lost while the Web UI's TaskManager page keeps polling it, or when following a link from a job's exception history) returned HTTP 500 with anUnhandled exceptionERROR log on the JobManager, and the Web UI kept re-requesting it indefinitely.This returns a proper 404 for a gone TaskManager and stops the futile polling, showing a clear "TaskManager not available" notice instead of an endless skeleton.
This supersedes #22542 (thanks @JunRuiLee for the original investigation). That PR addressed FLINK-31931 by preventing the exception-history page from linking to a gone TaskManager. Handling the not-found at the destination instead is more robust: it also covers deep-links and a TaskManager dying while its page is open, it is not racy (a TaskManager can disappear right after the link is rendered), and it keeps the link informative for the common case where the exception's TaskManager is already gone by the time the history is viewed.
Brief change log
TaskManagerDetailsHandlerstrips the failure withstripCompletionException(notstripExecutionException), so theUnknownTaskExecutorExceptiontoNOT_FOUNDmapping actually fires; this matches the sibling TaskManager handlers. Previously a gone TaskManager produced a 500 plus a repeatedUnhandled exceptionERROR log.loadManagerno longer swallows errors; a shared poll helper stops polling once the backend reports the TaskManager is gone (404), while transient errors keep retrying. The TaskManager status view shows a "TaskManager not available" notice and the metric cards are hidden instead of rendering blank tables.Verifying this change
TaskManagerDetailsHandlerTest#testUnknownTaskExecutorLeadsToNotFoundasserts a 404 for an unknown TaskManager id.Unhandled exceptionERROR log on the JobManager, versus the previous endless 500s.Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
AI Usage Disclosure