[FLINK-16303][rest] Enable retrieval of custom JobManager log files #11542

jerry-024 · 2020-03-27T04:24:15Z

What is the purpose of the change

This pull request makes rest API could get log list and get the log by custom name for jobmanager.

Brief change log

get log list
get the log by name

Verifying this change

This change added tests and can be verified as follows:

Added JobManagerLogListHandlerTest that verfied JobManagerLogListHandler.
Added WebFrontendITCase.getCustomLogFiles could verfied JobManagerCustomLogHandler.

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): ( no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn/Mesos, ZooKeeper: (no )
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (yes)
If yes, how is the feature documented? (docs)

flinkbot · 2020-03-27T04:26:19Z

Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community
to review your pull request. We will use this comment to track the progress of the review.

Automated Checks

Last check on commit 6c59031 (Fri Mar 27 04:26:19 UTC 2020)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!

_{Mention the bot in a comment to re-run the automated checks.}

Review Progress

❓ 1. The [description] looks good.
❓ 2. There is [consensus] that the contribution should go into to Flink.
❓ 3. Needs [attention] from.
❓ 4. The change fits into the overall [architecture].
❓ 5. Overall code [quality] is good.

Please see the Pull Request Review Guide for a full explanation of the review process.

The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot approve description to approve one or more aspects (aspects: description, consensus, architecture and quality)
@flinkbot approve all to approve all aspects
@flinkbot approve-until architecture to approve everything until architecture
@flinkbot attention @username1 [@username2 ..] to require somebody's attention
@flinkbot disapprove architecture to remove an approval you gave earlier

flinkbot · 2020-03-27T04:45:03Z

CI report:

c45cb2a UNKNOWN
bcef6e2 Travis: SUCCESS Azure: FAILURE

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run travis re-run the last Travis build
@flinkbot run azure re-run the last Azure build

jerry-024 · 2020-03-27T09:38:13Z

@flinkbot run travis

.../src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerCustomLogHandler.java

...me/src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandler.java

…ler,JobManagerLogListHandler

...me/src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandler.java

...rc/test/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandlerTest.java

GJL · 2020-03-31T15:30:49Z

For future reference, I think it would have been better to split this ticket into two:

Implement JobManagerLogListHandler
The rest

Separate commits also wouldn't hurt.

jerry-024 · 2020-04-01T08:33:19Z

For future reference, I think it would have been better to split this ticket into two:

Implement JobManagerLogListHandler

The rest

Separate commits also wouldn't hurt.

I don't agree to split this ticket into two.
The purpose of adding JobManagerLogListHandler is to allow the user to view the logs by custom.

GJL · 2020-04-01T10:16:50Z

I don't agree to split this ticket into two.
The purpose of adding JobManagerLogListHandler is to allow the user to view the logs by custom.

From my point of view, JobManagerLogListHandler lists the available log files. The ability to view the contents of the log files is a different story. Could we have implemented JobManagerLogListHandler independently from the other work in this PR? If yes, then I think we should have done it because it makes the review easier. For a Flink user it doesn't make a difference since the Web UI is not yet ready. I am not saying we should split the ticket now but for future tasks this is something to consider.

jerry-024 · 2020-04-01T11:47:36Z

I don't agree to split this ticket into two.
The purpose of adding JobManagerLogListHandler is to allow the user to view the logs by custom.

From my point of view, JobManagerLogListHandler lists the available log files. The ability to view the contents of the log files is a different story. Could we have implemented JobManagerLogListHandler independently from the other work in this PR? If yes, then I think we should have done it because it makes the review easier. For a Flink user it doesn't make a difference since the Web UI is not yet ready. I am not saying we should split the ticket now but for future tasks this is something to consider.

OK, I get your point. Thank you for your suggestion. I will pay attention to this later to make review easier.

...e/src/main/java/org/apache/flink/runtime/rest/messages/cluster/JobManagerLogListHeaders.java

flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorEndpoint.java

...time/src/main/java/org/apache/flink/runtime/rest/messages/cluster/FileMessageParameters.java

...c/main/java/org/apache/flink/runtime/rest/handler/cluster/AbstractJobManagerFileHandler.java

...me/src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandler.java

...c/main/java/org/apache/flink/runtime/rest/handler/cluster/AbstractJobManagerFileHandler.java

…JobManagerLogListHandler, FileMessageParameters, JobManagerLogListHeaders

GJL

Looking mostly good. I have left some minor suggestions.

...c/main/java/org/apache/flink/runtime/rest/handler/cluster/AbstractJobManagerFileHandler.java

GJL · 2020-04-03T10:29:18Z

...src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerStdoutFileHandler.java

+
+	@Override
+	protected File getFile(HandlerRequest<EmptyRequestBody, EmptyMessageParameters> handlerRequest) {
+		return this.logFileLocation.stdOutFile;


I find it strange that JobManagerStdoutFileHandler also knows about the location of the regular log file (i.e., LogFileLocation#logFile). Moreover, we are introducing a class hierarchy without adding behavior, i.e., JobManagerStdoutFileHandler and JobManagerLogFileHandler are doing the same thing (serving a (log) file).

I would propose to drop JobManagerStdoutFileHandler and add a File field to JobManagerLogFileHandler.
JobManagerCustomLogHandler should have a log dir field.

What do you think?

flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorEndpoint.java

...rc/test/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandlerTest.java

...me/src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandler.java

.../src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerCustomLogHandler.java

flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorUtils.java

...rc/test/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandlerTest.java

...c/main/java/org/apache/flink/runtime/rest/handler/cluster/AbstractJobManagerFileHandler.java

flink-runtime/src/main/java/org/apache/flink/runtime/webmonitor/WebMonitorUtils.java

GJL · 2020-04-13T20:34:21Z

Can we create a new issue for the Web UI to recreate this behavior? After this PR nothing will be shown if no log file is configured.

cc: @vthinkxie

GJL · 2020-04-13T22:19:13Z

...rc/test/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandlerTest.java

+				LogInfo logInfo = logInfo2Content.getKey();
+				String content = logInfo2Content.getValue();
+				File file = temporaryFolder.newFile(logInfo.getName());
+				FileUtils.writeStringToFile(file, content);


We should make sure that the file is UTF-8 or ASCII encoded otherwise we risk that one ASCII character is not one byte. writeStringToFile uses the system's default character set. I fixed it before merging.

I agree with you.
I find the same code in WebFrontendITCase, whether need we to fix them?

vthinkxie · 2020-04-16T02:28:35Z

Can we create a new issue for the Web UI to recreate this behavior? After this PR nothing will be shown if no log file is configured.

cc: @vthinkxie

adding the tips with (file unavailable) in the frontend code? I'm ok with this.

…r log files This closes apache#11542.

rmetzger added the review=description? label Mar 27, 2020

rmetzger added the component=Runtime/REST label Mar 27, 2020

jerry-024 force-pushed the FLINK-16303 branch from 234d59c to d00a917 Compare March 27, 2020 06:23

[FLINK-16303][rest] Enable retrieval of custom JobManager log files

380820d

jerry-024 force-pushed the FLINK-16303 branch from c45cb2a to 380820d Compare March 27, 2020 09:37

GJL self-assigned this Mar 27, 2020

hejianchao reviewed Mar 27, 2020

View reviewed changes

.../src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerCustomLogHandler.java Outdated Show resolved Hide resolved

hejianchao reviewed Mar 27, 2020

View reviewed changes

...me/src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerLogListHandler.java Outdated Show resolved Hide resolved

jerry-024 added 2 commits March 30, 2020 19:51

refactor LogFileNamePathParameter and update TaskManagerCustomLogHand…

2a61cc1

…ler,JobManagerLogListHandler

refactor LogFileNamePathParameter and update TaskManagerCustomLogHand…

31dd0d4

…ler,JobManagerLogListHandler

GJL reviewed Mar 31, 2020

View reviewed changes

update JobManagerLogListHandlerTest.createLogFileLocation

05f2fcf

jerry-024 force-pushed the FLINK-16303 branch from e980980 to 05f2fcf Compare April 1, 2020 09:27

GJL requested changes Apr 1, 2020

View reviewed changes

delete ConstantTextHandler and update AbstractJobManagerFileHandler, …

74a2a88

…JobManagerLogListHandler, FileMessageParameters, JobManagerLogListHeaders

GJL reviewed Apr 3, 2020

View reviewed changes

.../src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerCustomLogHandler.java Outdated Show resolved Hide resolved

GJL reviewed Apr 3, 2020

View reviewed changes

.../src/main/java/org/apache/flink/runtime/rest/handler/cluster/JobManagerCustomLogHandler.java Outdated Show resolved Hide resolved

delete JobManagerStdoutFileHandler and add logDir for LogFileLocation

3d1ea44

GJL reviewed Apr 8, 2020

View reviewed changes

Update logDir type from string to File and update test

587d84b

jerry-024 requested a review from GJL April 9, 2020 01:23

update JobManagerLogListHandlerTest

bcef6e2

GJL mentioned this pull request Apr 13, 2020

[FLINK-16303][rest] Enable retrieval of custom JobManager log files #11720

Merged

GJL closed this in 53755ec Apr 13, 2020

GJL reviewed Apr 13, 2020

View reviewed changes

jrthe42 added a commit to jrthe42/flink that referenced this pull request May 13, 2020

cherry-pick: [FLINK-16303][rest] Enable retrieval of custom JobManage…

d494041

…r log files This closes apache#11542.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-16303][rest] Enable retrieval of custom JobManager log files #11542

[FLINK-16303][rest] Enable retrieval of custom JobManager log files #11542

jerry-024 commented Mar 27, 2020

flinkbot commented Mar 27, 2020

flinkbot commented Mar 27, 2020 •

edited

jerry-024 commented Mar 27, 2020

GJL commented Mar 31, 2020 •

edited

jerry-024 commented Apr 1, 2020

GJL commented Apr 1, 2020 •

edited

jerry-024 commented Apr 1, 2020

GJL left a comment

GJL Apr 3, 2020

GJL commented Apr 13, 2020 •

edited

GJL Apr 13, 2020 •

edited

jerry-024 Apr 15, 2020

vthinkxie commented Apr 16, 2020 •

edited

[FLINK-16303][rest] Enable retrieval of custom JobManager log files #11542

[FLINK-16303][rest] Enable retrieval of custom JobManager log files #11542

Conversation

jerry-024 commented Mar 27, 2020

What is the purpose of the change

Brief change log

Verifying this change

Does this pull request potentially affect one of the following parts:

Documentation

flinkbot commented Mar 27, 2020

Automated Checks

Review Progress

flinkbot commented Mar 27, 2020 • edited

CI report:

jerry-024 commented Mar 27, 2020

GJL commented Mar 31, 2020 • edited

jerry-024 commented Apr 1, 2020

GJL commented Apr 1, 2020 • edited

jerry-024 commented Apr 1, 2020

GJL left a comment

Choose a reason for hiding this comment

GJL Apr 3, 2020

Choose a reason for hiding this comment

GJL commented Apr 13, 2020 • edited

GJL Apr 13, 2020 • edited

Choose a reason for hiding this comment

jerry-024 Apr 15, 2020

Choose a reason for hiding this comment

vthinkxie commented Apr 16, 2020 • edited

flinkbot commented Mar 27, 2020 •

edited

GJL commented Mar 31, 2020 •

edited

GJL commented Apr 1, 2020 •

edited

GJL commented Apr 13, 2020 •

edited

GJL Apr 13, 2020 •

edited

vthinkxie commented Apr 16, 2020 •

edited