Airblader commented on a change in pull request #18868:
URL: https://github.com/apache/flink/pull/18868#discussion_r811736714
##########
File path: flink-runtime-web/web-dashboard/src/app/pages/job/job.component.ts
##########
@@ -49,6 +52,9 @@ export class JobComponent implements OnInit, OnDestroy {
takeUntil(this.destroy$),
mergeMap(() =>
this.jobService.loadJob(this.activatedRoute.snapshot.params.jid).pipe(
+ tap(job => {
+ this.jobLocalService.setJobDetail(job);
+ }),
Review comment:
We don't need to resolve this here, but I think this is going the wrong
way: the service should be providing the data and flow it into the components,
not the component loading the data and pushing it into a service.
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/job-local.service.ts
##########
@@ -0,0 +1,53 @@
+import { Injectable } from '@angular/core';
+import { combineLatest, Observable, ReplaySubject } from 'rxjs';
+import { filter, map } from 'rxjs/operators';
+
+import { JobDetailCorrect, NodesItemCorrect } from 'interfaces';
+
+type Nullable<T> = T | null;
+
+interface JobWithVertex {
Review comment:
I think we should export types that are referenced in public APIs,
otherwise the consuming code cannot reference the type.
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/overview/job-overview.component.ts
##########
@@ -109,13 +116,28 @@ export class JobOverviewComponent implements OnInit,
OnDestroy {
} else {
this.dagreComponent.focusNode(this.selectedNode, true);
}
+ this.cdr.markForCheck();
+ }
+
+ public refreshNodesWithMetrics(): void {
+ this.mergeWithBackPressure(this.nodes)
+ .pipe(
+ mergeMap(nodes => this.mergeWithWatermarks(nodes)),
+ takeUntil(this.destroy$)
+ )
+ .subscribe(nodes2 => {
Review comment:
nit: the `2` is unnecessary.
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/job-local.service.ts
##########
@@ -0,0 +1,53 @@
+import { Injectable } from '@angular/core';
+import { combineLatest, Observable, ReplaySubject } from 'rxjs';
+import { filter, map } from 'rxjs/operators';
+
+import { JobDetailCorrect, NodesItemCorrect } from 'interfaces';
+
+type Nullable<T> = T | null;
Review comment:
This is only used once, not shorter than what it encapsulates, and adds
an indirection to an unexported type in a public API – I would remove this type
and just write `| null`.
##########
File path: flink-runtime-web/web-dashboard/src/app/interfaces/apis.ts
##########
@@ -0,0 +1,159 @@
+import { HttpEvent } from '@angular/common/http';
+import { Observable } from 'rxjs';
+
+import {
+ Checkpoint,
+ CheckpointConfig,
+ CheckpointDetail,
+ CheckpointSubTask,
+ JobAccumulators,
+ JobBackpressure,
+ JobConfig,
+ JobDetailCorrect,
+ JobException,
+ JobFlameGraph,
+ JobManagerLogItem,
+ JobsItem,
+ JobSubTask,
+ JobSubTaskTime,
+ JobVertexTaskManager,
+ JobMetric,
+ MetricMap,
+ MetricMapWithTimestamp,
+ Overview,
+ TaskManagerDetail,
+ TaskManagerLogItem,
+ TaskManagerLogDetail,
+ TaskManagersItem,
+ Watermarks,
+ JobManagerLogDetail,
+ JobManagerConfig,
+ JarList,
+ PlanDetail
+} from 'interfaces';
+
+export interface APIService {
+ getUrlPrefix(): string;
+}
+
+export abstract class OverviewAPIService implements APIService {
Review comment:
What's the purpose of extracting these base classes? They all just have
one implementation. They currently just duplicate the structure (and
unnecessarily so since TypeScript is structurally typed anyway), and do so in a
location far away from the actual implementation.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
