[FLINK-17130][web] Enable listing JM Logs and displaying Logs by filename #11731

vthinkxie · 2020-04-14T09:31:07Z

What is the purpose of the change

ref https://issues.apache.org/jira/browse/FLINK-17130

Brief change log

add JM log list
add JM log detail

Verifying this change

the job manager log list
the job manager log detail
check the download button
check the reload button
check the fullscreen button

before:

after:

list:

log-detail from the list:

full screen:

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, Yarn/Mesos, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

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

…name

flinkbot · 2020-04-14T09:34:38Z

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 2a9f46b (Tue Apr 14 09:34:38 UTC 2020)

Warnings:

No documentation files were touched! Remember to keep the Flink docs up to date!
This pull request references an unassigned Jira ticket. According to the code contribution guide, tickets need to be assigned before starting with the implementation work.

_{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

vthinkxie · 2020-04-14T09:43:40Z

@simplejason do you have time to take a look?

flinkbot · 2020-04-14T09:47:44Z

CI report:

64ad347 Travis: FAILURE 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

simplejason

Everything works as expected, the PR looks good to me, I just left a few remarks.

simplejason · 2020-04-14T13:00:19Z

...web/web-dashboard/src/app/pages/job-manager/log-detail/job-manager-log-detail.component.less

+.breadcrumb {
+  background: @component-background;
+  border-bottom: 1px solid @border-color-split;
+  margin-bottom: 16px;
+  padding: 12px 24px;
  position: relative;
-  display: block;
+}
+
+flink-refresh-download {
+  position: absolute;
+  right: 12px;
+  top: 0;
+  line-height: 47px;


We seem to have done some duplicate work with Task Managers log list, can we make these codes reusable?

these two components look very similar, but the style is not the same(some border and full-screen style need to adjust according to different layout)
and they may have more different features in the future, like #10228, I think it would better not to create a share component between them

simplejason · 2020-04-14T13:06:22Z

...web/web-dashboard/src/app/pages/job-manager/log-detail/job-manager-log-detail.component.html

+    (reload)="reload()"
+    (fullScreen)="toggleFullScreen($event)">
+  </flink-refresh-download>
+</div>
 <flink-monaco-editor [value]="logs"></flink-monaco-editor>


When I scroll the log content, a border appears at the top of the editor, I think it would be nice to hide it, but it's also ok if we want to keep it :)

simplejason

LGTM

GJL · 2020-04-27T10:05:05Z

Feature works as expected but I have some remarks regarding the usability.

If a user starts and stops the cluster multiple times locally, the log list gets confusing quickly, i.e., it is difficult to find the latest/active log file. See screenshot below:

You can simulate this yourself by running:

for i in {0..10} do; bin/start-cluster.sh && bin/stop-cluster.sh done

For task manager logs it is similar but perhaps a bit better since there is a way to access the "main TM log" directly from the sub vertex page.

Do you have any ideas how this can be improved? FLINK-16863 would not fully solve the above problem; if a user has many local TMs running, it could still be difficult to find the current/active JM log.

cc: @jinglining

vthinkxie · 2020-04-27T11:45:50Z

Hi @GJL

I have a discussion with @jinglining , what about the following solutions?

split the taskmanager and jobmanager logs into different folders to avoid they mixed together in the standalone mode (which may bring break changes?).
add lastModified describe in FLINK-16863 and current:boolean data in the list item.

the current means that the log has the same name as the previous API such as /jobmanager/stdout, I can add a highlight or a tag in the frontend to prompt the users with it.

we can get it from

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

Lines 71 to 75 in b41f09f

    
           public static class LogFileLocation { 
        
           	public final File logFile; 
        
           	public final File stdOutFile; 
        
           	public final File logDir;

GJL · 2020-04-27T15:12:28Z

First of all, I am sorry that I did not see this problem earlier.

split the taskmanager and jobmanager logs into different folders to avoid they mixed together in the standalone mode (which may bring break changes?).

add lastModified describe in FLINK-16863 and current:boolean data in the list item.

It would mitigate the issue. However, the user would still see additional log files belonging to other processes, e.g., when users are running multiple standalone TMs on the same host. Ideally, only files owned by the respective TM/JM process are listed. Therefore, an extension of your idea that @tillrohrmann had is to put all logs belonging to a single process into a separate directory. However, for a user who wants to configure custom log files, it could be difficult to predict the name of the log directory, which would render the whole feature useless.

A (temporary) workaround could be to always show the main log and stdout file by default and make the log list a separate page. This would allow us to roll out the feature while not changing existing behavior.

I think we should not make a decision on our own. Maybe it is acceptable to see other processes' log files. How about we bring this issue up on the dev mailing list and see what others are saying (especially the developers that voted on FLIP-103 in the first place)?

vthinkxie · 2020-04-28T02:05:00Z

I have bring up this issue in the mail list
https://lists.apache.org/thread.html/rdb47e80cd398bb4086f31511e0a2ad46a67a9c20d539bd872df2d2c0%40%3Cdev.flink.apache.org%3E

vthinkxie · 2020-05-11T11:47:53Z

I have revert the deletion of the sdtout and logs page
cc @GJL

simplejason

LGTM.
Stdout and Logs work for me.

…name This closes apache#11731.

[FLINK-17130][web] Enable listing JM Logs and displaying Logs by file…

a68b809

…name

vthinkxie force-pushed the FLINK-17130 branch from 2a9f46b to a68b809 Compare April 14, 2020 09:34

rmetzger added the review=description? label Apr 14, 2020

rmetzger added the component=Runtime/WebFrontend label Apr 14, 2020

simplejason reviewed Apr 14, 2020

View reviewed changes

GJL self-assigned this Apr 21, 2020

[FLINK-17130][web] remove margin-bottom when full screen

8445659

simplejason approved these changes Apr 23, 2020

View reviewed changes

[FLINK-17130][web] add logs and stdout back

64ad347

simplejason approved these changes May 11, 2020

View reviewed changes

GJL approved these changes May 12, 2020

View reviewed changes

GJL closed this in 74b850c May 12, 2020

klion26 pushed a commit to klion26/flink that referenced this pull request May 21, 2020

[FLINK-17130][web] Enable listing JM logs and displaying logs by file…

acb5424

…name This closes apache#11731.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FLINK-17130][web] Enable listing JM Logs and displaying Logs by filename #11731

[FLINK-17130][web] Enable listing JM Logs and displaying Logs by filename #11731

vthinkxie commented Apr 14, 2020

flinkbot commented Apr 14, 2020

vthinkxie commented Apr 14, 2020

flinkbot commented Apr 14, 2020 •

edited

Loading

simplejason left a comment

simplejason Apr 14, 2020

vthinkxie Apr 23, 2020

simplejason Apr 14, 2020

vthinkxie Apr 23, 2020 •

edited

Loading

simplejason left a comment

GJL commented Apr 27, 2020 •

edited

Loading

vthinkxie commented Apr 27, 2020 •

edited

Loading

GJL commented Apr 27, 2020 •

edited

Loading

vthinkxie commented Apr 28, 2020

vthinkxie commented May 11, 2020 •

edited

Loading

simplejason left a comment

[FLINK-17130][web] Enable listing JM Logs and displaying Logs by filename #11731

[FLINK-17130][web] Enable listing JM Logs and displaying Logs by filename #11731

Conversation

vthinkxie commented Apr 14, 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 Apr 14, 2020

Automated Checks

Review Progress

vthinkxie commented Apr 14, 2020

flinkbot commented Apr 14, 2020 • edited Loading

CI report:

simplejason left a comment

Choose a reason for hiding this comment

simplejason Apr 14, 2020

Choose a reason for hiding this comment

vthinkxie Apr 23, 2020

Choose a reason for hiding this comment

simplejason Apr 14, 2020

Choose a reason for hiding this comment

vthinkxie Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

simplejason left a comment

Choose a reason for hiding this comment

GJL commented Apr 27, 2020 • edited Loading

vthinkxie commented Apr 27, 2020 • edited Loading

GJL commented Apr 27, 2020 • edited Loading

vthinkxie commented Apr 28, 2020

vthinkxie commented May 11, 2020 • edited Loading

simplejason left a comment

Choose a reason for hiding this comment

flinkbot commented Apr 14, 2020 •

edited

Loading

vthinkxie Apr 23, 2020 •

edited

Loading

GJL commented Apr 27, 2020 •

edited

Loading

vthinkxie commented Apr 27, 2020 •

edited

Loading

GJL commented Apr 27, 2020 •

edited

Loading

vthinkxie commented May 11, 2020 •

edited

Loading