You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@flink.apache.org by GitBox <gi...@apache.org> on 2022/05/23 06:50:14 UTC
[GitHub] [flink] Airblader commented on a diff in pull request #19712: [FLINK-26207][runtime-web] make web ui's components depend on injected configurable object
Airblader commented on code in PR #19712:
URL: https://github.com/apache/flink/pull/19712#discussion_r879062241
##########
flink-runtime-web/web-dashboard/src/app/app.component.less:
##########
@@ -137,9 +137,9 @@ nz-header {
}
nz-content {
+ display: flex;
Review Comment:
These changes are unrelated to the config object change. Given the size of the PR, can you please at least split this (and all other unrelated changes) into logical, unrelated commits? We can still do it in one PR.
##########
flink-runtime-web/web-dashboard/src/app/pages/job-manager/configuration/job-manager-configuration.component.ts:
##########
@@ -26,15 +29,33 @@ import { JobManagerService } from '@flink-runtime-web/services';
styleUrls: ['./job-manager-configuration.component.less'],
changeDetection: ChangeDetectionStrategy.OnPush
})
-export class JobManagerConfigurationComponent implements OnInit {
+export class JobManagerConfigurationComponent implements OnInit, OnDestroy {
public listOfConfig: Array<{ key: string; value: string }> = [];
+ public loading = true;
+ private destroy$ = new Subject<void>();
+
+ public readonly trackByConfig = (_: number, value: JobManagerConfig): string => {
+ return value.key;
+ };
constructor(private readonly jobManagerService: JobManagerService, private readonly cdr: ChangeDetectorRef) {}
public ngOnInit(): void {
- this.jobManagerService.loadConfig().subscribe(data => {
- this.listOfConfig = data.sort((pre, next) => (pre.key > next.key ? 1 : -1));
- this.cdr.markForCheck();
- });
+ this.jobManagerService
+ .loadConfig()
+ .pipe(
+ catchError(() => of([] as JobManagerConfig[])),
+ takeUntil(this.destroy$)
+ )
+ .subscribe(listOfConfig => {
+ this.listOfConfig = listOfConfig.sort((a, b) => a.key.localeCompare(b.key));
Review Comment:
I assume `key` can never be undefined/null, correct? Otherwise this could throw an error that it didn't use to throw.
##########
flink-runtime-web/web-dashboard/src/app/pages/job-manager/thread-dump/job-manager-thread-dump.component.ts:
##########
@@ -31,16 +35,26 @@ import { EditorOptions } from 'ng-zorro-antd/code-editor/typings';
changeDetection: ChangeDetectionStrategy.OnPush
})
export class JobManagerThreadDumpComponent implements OnInit, OnDestroy {
- public readonly editorOptions: EditorOptions = flinkEditorOptions;
-
+ public editorOptions: EditorOptions;
public dump = '';
public loading = true;
+ public downloadUrl = '';
+ public downloadName = '';
private readonly destroy$ = new Subject<void>();
- constructor(private readonly jobManagerService: JobManagerService, private readonly cdr: ChangeDetectorRef) {}
+ constructor(
+ private readonly jobManagerService: JobManagerService,
+ private readonly configService: ConfigService,
+ private readonly cdr: ChangeDetectorRef,
+ @Inject(JOB_MANAGER_MODULE_CONFIG) readonly moduleConfig: JobManagerModuleConfig
+ ) {
+ this.editorOptions = moduleConfig.editorOptions || JOB_MANAGER_MODULE_DEFAULT_CONFIG.editorOptions;
+ }
public ngOnInit(): void {
+ this.downloadUrl = `${this.configService.BASE_URL}/jobmanager/thread-dump`;
+ this.downloadName = `jobmanager_thread_dump`;
Review Comment:
These can be assigned at their declaration directly (and could be made `readonly` then).
##########
flink-runtime-web/web-dashboard/src/app/pages/job-manager/job-manager.config.ts:
##########
@@ -0,0 +1,41 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { InjectionToken } from '@angular/core';
+
+import { flinkEditorOptions } from '@flink-runtime-web/share/common/editor/editor-config';
+import { EditorOptions } from 'ng-zorro-antd/code-editor/typings';
+
+export interface JobManagerModuleConfig {
+ editorOptions?: EditorOptions;
+ jobManagerLogRouterFactory?: (...args: string[]) => string | string[];
+}
+
+export const JOB_MANAGER_MODULE_DEFAULT_CONFIG: Required<JobManagerModuleConfig> = {
+ editorOptions: flinkEditorOptions,
+ jobManagerLogRouterFactory: jobManagerName => [jobManagerName]
+};
+
+export const JOB_MANAGER_MODULE_CONFIG = new InjectionToken<JobManagerModuleConfig>('job-manager-module-config', {
+ providedIn: 'root',
+ factory: JOB_MANAGER_MODULE_CONFIG_FACTORY
+});
+
+export function JOB_MANAGER_MODULE_CONFIG_FACTORY(): JobManagerModuleConfig {
Review Comment:
This doesn't need to be exported, I think.
##########
flink-runtime-web/web-dashboard/src/app/pages/job-manager/log-detail/job-manager-log-detail.component.ts:
##########
@@ -35,20 +40,24 @@ import { EditorOptions } from 'ng-zorro-antd/code-editor/typings';
styleUrls: ['./job-manager-log-detail.component.less']
})
export class JobManagerLogDetailComponent implements OnInit, OnDestroy {
+ public editorOptions: EditorOptions;
public logs = '';
public logName = '';
public downloadUrl = '';
public isLoading = false;
public isFullScreen = false;
- public editorOptions: EditorOptions = flinkEditorOptions;
private readonly destroy$ = new Subject<void>();
constructor(
private readonly jobManagerService: JobManagerService,
private readonly cdr: ChangeDetectorRef,
- private readonly activatedRoute: ActivatedRoute
- ) {}
+ private readonly activatedRoute: ActivatedRoute,
+ private readonly configService: ConfigService,
+ @Inject(JOB_MANAGER_MODULE_CONFIG) readonly moduleConfig: JobManagerModuleConfig
+ ) {
+ this.editorOptions = moduleConfig.editorOptions || JOB_MANAGER_MODULE_DEFAULT_CONFIG.editorOptions;
Review Comment:
Why do we need this fallback to the non-injected version?
##########
flink-runtime-web/web-dashboard/src/app/pages/job-manager/log-list/job-manager-log-list.component.html:
##########
@@ -16,38 +16,40 @@
~ limitations under the License.
-->
-<nz-card [nzBordered]="false">
- <nz-table
- [nzSize]="'small'"
- [nzData]="listOfLog"
- [nzLoading]="isLoading"
- [nzFrontPagination]="false"
- [nzShowPagination]="false"
- [nzVirtualItemSize]="39"
- [nzVirtualForTrackBy]="trackByName"
- [nzVirtualMinBufferPx]="480"
- [nzVirtualMaxBufferPx]="480"
- [nzScroll]="{ y: '650px' }"
- >
- <thead>
- <tr>
- <th nzWidth="50%">Log Name</th>
- <th [nzSortFn]="sortLastModifiedTimeFn">Last Modified Time</th>
- <th [nzSortFn]="sortSizeFn">Size (KB)</th>
- </tr>
- </thead>
- <tbody>
- <ng-template nz-virtual-scroll let-data>
- <ng-container *ngIf="narrowLogData(data) as narrowData">
- <tr>
- <td>
- <a [routerLink]="[narrowData.name]">{{ narrowData.name }}</a>
- </td>
- <td>{{ narrowData.mtime | humanizeDate: 'yyyy-MM-dd HH:mm:ss' }}</td>
- <td>{{ narrowData.size / 1024 | number: '1.0-2' }}</td>
- </tr>
- </ng-container>
- </ng-template>
- </tbody>
- </nz-table>
-</nz-card>
+<nz-table
+ nzBordered
+ [nzSize]="'small'"
Review Comment:
nit: all `[foo]="'bar'"` could just be written as `foo="bar"`.
##########
flink-runtime-web/web-dashboard/src/app/pages/job/overview/accumulators/job-overview-drawer-accumulators.component.html:
##########
@@ -34,11 +38,13 @@
</tr>
</thead>
<tbody>
- <tr *ngFor="let accumulator of listOfAccumulator; trackBy: trackByName">
- <td>{{ accumulator.name }}</td>
- <td>{{ accumulator.type }}</td>
- <td>{{ (accumulator.value | number: '1.0-3') || accumulator.value }}</td>
- </tr>
+ <ng-template nz-virtual-scroll let-data>
+ <tr>
Review Comment:
Can we use a `typDefinition()` here to enforce the types a bit?
##########
flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.less:
##########
@@ -35,8 +40,8 @@
}
nz-code-editor {
- position: relative;
- height: calc(~"100vh - 310px");
+ position: absolute;
Review Comment:
Have you tried this in relevant browsers? I would have expected this to need something like
```
top: 0;
left: 0;
bottom: 0;
right: 0;
```
Or does nz-code-editor detect and handle this?
##########
flink-runtime-web/web-dashboard/src/app/pages/job-manager/log-detail/job-manager-log-detail.component.ts:
##########
@@ -66,6 +75,12 @@ export class JobManagerLogDetailComponent implements OnInit, OnDestroy {
this.jobManagerService
.loadLog(this.logName)
.pipe(
+ catchError(() => {
Review Comment:
What is the reason for this change? What does it have to do with the overall PR?
##########
flink-runtime-web/web-dashboard/src/app/pages/job-manager/log-list/job-manager-log-list.component.less:
##########
@@ -19,7 +19,26 @@
@import "theme";
:host {
- ::ng-deep .ant-card-body {
- padding: 0;
+ display: flex;
+ flex: 1;
+ padding: @padding-md;
+
+ ::ng-deep {
+ ::-webkit-scrollbar {
+ display: none;
+ }
+
+ nz-table,
+ nz-spin,
+ cdk-virtual-scroll-viewport,
+ nz-table-inner-scroll,
+ .ant-spin-container,
+ .ant-table {
+ height: 100%;
+ }
Review Comment:
(This all seems very hacky and fragile. It'd be great of ng-zorro exposed an API to achieve this…)
##########
flink-runtime-web/web-dashboard/src/app/pages/job/overview/accumulators/job-overview-drawer-accumulators.component.html:
##########
@@ -47,28 +53,34 @@
[nzSize]="'small'"
[nzLoading]="isLoading"
[nzData]="listOfSubTaskAccumulator"
- [nzScroll]="{ y: 'calc( 100vh - 72px )' }"
+ [nzScroll]="{ y: '100%' }"
[nzFrontPagination]="false"
[nzShowPagination]="false"
+ [nzVirtualItemSize]="virtualItemSize"
+ [nzVirtualMinBufferPx]="300"
+ [nzVirtualMaxBufferPx]="300"
+ [nzVirtualForTrackBy]="trackByName"
>
<thead>
<tr>
- <th nzWidth="25%" nzLeft="0px">SubTask</th>
+ <th nzWidth="25%" nzLeft>SubTask</th>
<th nzWidth="25%">Name</th>
<th nzWidth="25%">Type</th>
- <th nzWidth="25%" nzRight="0px">Value</th>
+ <th nzWidth="25%" nzRight>Value</th>
</tr>
</thead>
<tbody>
- <tr *ngFor="let data of listOfSubTaskAccumulator; trackBy: trackByName">
- <td nzLeft="0px">
- ({{ data.subtask }}) {{ data.host }}, attempt:
- {{ data.attempt + 1 }}
- </td>
- <td>{{ data.name }}</td>
- <td>{{ data.type }}</td>
- <td nzRight="0px">{{ data.value }}</td>
- </tr>
+ <ng-template nz-virtual-scroll let-data>
+ <tr>
Review Comment:
`typeDefinition()` here, too.
##########
flink-runtime-web/web-dashboard/src/app/pages/job/overview/job-overview.config.ts:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { InjectionToken, Type } from '@angular/core';
+
+import {
+ JobOverviewSubtasksTableAction,
+ SubtasksTableActionComponent
+} from '@flink-runtime-web/pages/job/overview/subtasks/table-action/subtasks-table-action.component';
+import {
+ JobOverviewTaskManagersTableAction,
+ TaskmanagersTableActionComponent
+} from '@flink-runtime-web/pages/job/overview/taskmanagers/table-action/taskmanagers-table-action.component';
+import { BackpressureBadgeComponent } from '@flink-runtime-web/share/customize/backpressure-badge/backpressure-badge.component';
+import { JobBadgeComponent } from '@flink-runtime-web/share/customize/job-badge/job-badge.component';
+import { TaskBadgeComponent } from '@flink-runtime-web/share/customize/task-badge/task-badge.component';
+
+export interface JobOverviewModuleConfig {
+ taskManagerActionComponent?: Type<JobOverviewTaskManagersTableAction>;
+ subtaskActionComponent?: Type<JobOverviewSubtasksTableAction>;
+ stateBadgeComponent?: Type<unknown>;
+ taskCountBadgeComponent?: Type<unknown>;
+ backpressureBadgeComponent?: Type<unknown>;
Review Comment:
These configurations add a lot of complexity, but do not improve any reusability within Flink. It essentially turns Flink UI into its own public API.
##########
flink-runtime-web/web-dashboard/src/app/pages/job/overview/job-overview.config.ts:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { InjectionToken, Type } from '@angular/core';
+
+import {
+ JobOverviewSubtasksTableAction,
+ SubtasksTableActionComponent
+} from '@flink-runtime-web/pages/job/overview/subtasks/table-action/subtasks-table-action.component';
+import {
+ JobOverviewTaskManagersTableAction,
+ TaskmanagersTableActionComponent
+} from '@flink-runtime-web/pages/job/overview/taskmanagers/table-action/taskmanagers-table-action.component';
+import { BackpressureBadgeComponent } from '@flink-runtime-web/share/customize/backpressure-badge/backpressure-badge.component';
+import { JobBadgeComponent } from '@flink-runtime-web/share/customize/job-badge/job-badge.component';
+import { TaskBadgeComponent } from '@flink-runtime-web/share/customize/task-badge/task-badge.component';
+
+export interface JobOverviewModuleConfig {
+ taskManagerActionComponent?: Type<JobOverviewTaskManagersTableAction>;
+ subtaskActionComponent?: Type<JobOverviewSubtasksTableAction>;
+ stateBadgeComponent?: Type<unknown>;
+ taskCountBadgeComponent?: Type<unknown>;
+ backpressureBadgeComponent?: Type<unknown>;
+}
+
+export const JOB_OVERVIEW_MODULE_DEFAULT_CONFIG: Required<JobOverviewModuleConfig> = {
+ taskManagerActionComponent: TaskmanagersTableActionComponent,
+ subtaskActionComponent: SubtasksTableActionComponent,
+ stateBadgeComponent: JobBadgeComponent,
+ taskCountBadgeComponent: TaskBadgeComponent,
+ backpressureBadgeComponent: BackpressureBadgeComponent
+};
+
+export const JOB_OVERVIEW_MODULE_CONFIG = new InjectionToken<JobOverviewModuleConfig>('job-manager-module-config', {
+ providedIn: 'root',
+ factory: JOB_OVERVIEW_MODULE_CONFIG_FACTORY
+});
+
+export function JOB_OVERVIEW_MODULE_CONFIG_FACTORY(): JobOverviewModuleConfig {
Review Comment:
Doesn't need to be exported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@flink.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org