[FLINK-8503] [flip6] Display TaskExecutor logs and stdout files in web ui #5353
Conversation
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
tillrohrmann
force-pushed
the
enableTaskManagerLogRetrieval
branch
5 times, most recently
from
0b66d69 to
dac8d9b
Compare
GJL
reviewed
Jan 31, 2018
| * @param timeout for the asynchronous operation | ||
| * @return Future which is completed with the {@link TransientBlobKey} of the uploaded file. | ||
| */ | ||
| CompletableFuture<TransientBlobKey> requestFileUpload(FileType fileType, @RpcTimeout Time timeout); |
Member
There was a problem hiding this comment.
Don't we need to clean up these methods?
@Override
public CompletableFuture<TransientBlobKey> requestTaskManagerLog(Time timeout) {
// return taskExecutorGateway.requestTaskManagerLog(timeout);
throw new UnsupportedOperationException("Operation is not yet supported.");
}
@Override
public CompletableFuture<TransientBlobKey> requestTaskManagerStdout(Time timeout) {
// return taskExecutorGateway.requestTaskManagerStdout(timeout);
throw new UnsupportedOperationException("Operation is not yet supported.");
}
Contributor
Author
There was a problem hiding this comment.
Yes, we should adapt RpcTaskManagerGateway accordingly.
GJL
reviewed
Feb 5, 2018
| config, | ||
| handlerConfig, | ||
| resourceManagerGatewayRetriever, | ||
| transientBlobService, |
Member
There was a problem hiding this comment.
I don't think we need these static final variables. Can just use NoOpTransientBlobService.INSTANCE as a constructor argument.
Contributor
Author
There was a problem hiding this comment.
True, will change it.
| final InetSocketAddress currentServerAddress = serverAddress; | ||
|
|
||
| if (currentServerAddress != null) { | ||
| return new BlobClient(serverAddress, blobClientConfig); |
Member
There was a problem hiding this comment.
I think you should use currentServerAddress, or the creation can fail with an NPE.
Contributor
Author
There was a problem hiding this comment.
Definitely. Good catch!
GJL
requested changes
Feb 5, 2018
| (Void ignored, Throwable throwable) -> { | ||
| if (throwable != null) { | ||
| log.debug("Failed to transfer file from TaskExecutor {}.", taskManagerId, throwable); | ||
| fileBlobKeys.invalidate(taskManagerId); |
Member
There was a problem hiding this comment.
It's not enough to invalidate the cache, you must also send an error back to the client, or the HTTP request cannot be finished.
This will block forever if the TM is not registered:
curl -v http://localhost:9067/taskmanagers/daecac46c3f0f13b945fd2bb94438204/log
Contributor
Author
There was a problem hiding this comment.
This is true. Will add an error response here.
tillrohrmann
force-pushed
the
enableTaskManagerLogRetrieval
branch
from
4a27bd1 to
1be552e
Compare
tillrohrmann
added a commit
to tillrohrmann/flink
that referenced
this pull request
Feb 6, 2018
…b ui Introduce the AbstractHandler which takes a typed request and returns an untyped response. The AbstractRestHandler extends the AbstractHandler to add typed reponses. Introduce AbstractTaskManagerFileHandler which encapsulates the file loading logic. Upon request of a TaskManager file, the handler will trigger the file upload via the ResourceManager. The returned TransientBlobKey is then downloaded via the TransientBlobService. Once downloaded, the file is served to the client. Each transient blob key is cached for maximum duration after which it is purged and has to be reuploaded by the TaskExecutor. This closes apache#5353.
Contributor
Author
Instead of creating for each new JobManagerConnection a dedicated BlobCacheService the TaskExecutor uses a single BlobCacheService which it shares between the different JobManagerConnections. The initial BlobServer address is passed by the ResourceManager when the TaskExecutor registers at it. In order to avoid the re- creation of BlobCacheServices, this commit changes the behaviour such that one can update the BlobServer address. This closes apache#5350.
The JobManagerRunnerMockTest is completely ignored. Moreover, it tests things with heavy usage of Mockito which is hard to maintain.
This commit removes the LibraryCacheManager from the JobMaster since it is no longer needed. The JobMaster is started with the correct user code class loader and, thus, does not need the LibraryCacheManager. This commit also corrects that the BlobServer is not closed by the JobManagerServices#shutdown method. This closes apache#5352.
…b ui Introduce the AbstractHandler which takes a typed request and returns an untyped response. The AbstractRestHandler extends the AbstractHandler to add typed reponses. Introduce AbstractTaskManagerFileHandler which encapsulates the file loading logic. Upon request of a TaskManager file, the handler will trigger the file upload via the ResourceManager. The returned TransientBlobKey is then downloaded via the TransientBlobService. Once downloaded, the file is served to the client. Each transient blob key is cached for maximum duration after which it is purged and has to be reuploaded by the TaskExecutor. This closes apache#5353.
tillrohrmann
force-pushed
the
enableTaskManagerLogRetrieval
branch
from
1be552e to
be8feb8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
Introduce the AbstractHandler which takes a typed request and returns an untyped
response. The AbstractRestHandler extends the AbstractHandler to add typed reponses.
Introduce AbstractTaskManagerFileHandler which encapsulates the file loading logic.
Upon request of a TaskManager file, the handler will trigger the file upload via
the ResourceManager. The returned TransientBlobKey is then downloaded via the
TransientBlobService. Once downloaded, the file is served to the client. Each
transient blob key is cached for maximum duration after which it is purged and has
to be reuploaded by the TaskExecutor.
This PR is based on #5341
Brief change log
AbstractHandlerAbstractTaskManagerFileHandlerwhich is responsible for serving files from theTaskExecutorAbstractTaskManagerFileHandlertriggers the file upload via theResourceManagerwhich knows theTaskExecutors, additionally it caches theTransientBlobKeysin order to not always trigger a file uploadTaskManagerLogFileHandlerto serve the log fileTaskManagerStdoutFileHandlerto serve the stdout fileVerifying this change
AbstractTaskManagerFileHandlerTestDoes this pull request potentially affect one of the following parts:
@Public(Evolving): (no)Documentation