nicoweidner commented on a change in pull request #16807:
URL: https://github.com/apache/flink/pull/16807#discussion_r688618451
##########
File path: flink-runtime-web/web-dashboard/src/app/interfaces/job-exception.ts
##########
@@ -46,3 +46,7 @@ export interface ExceptionInfoInterface {
taskName: string;
location: string;
}
+
+export interface RootExceptionInfoInterface extends ExceptionInfoInterface {
Review comment:
export seems unnecessary
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.ts
##########
@@ -22,6 +22,12 @@ import { ExceptionInfoInterface } from 'interfaces';
import { distinctUntilChanged, flatMap, tap } from 'rxjs/operators';
import { JobService } from 'services';
+export interface ExceptionInfoInterfaceWrapper {
Review comment:
export seems unnecessary.
nit: What do you think of `ConcurrentExceptions`?
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.ts
##########
@@ -61,7 +67,22 @@ export class JobExceptionsComponent implements OnInit {
this.rootException = 'No Root Exception';
}
this.truncated = exceptionHistory.truncated;
- this.listOfException = exceptionHistory.entries;
+ this.listOfException = exceptionHistory.entries.map(entry => {
+ const values: ExceptionInfoInterface[] = [];
+ values.push(entry);
+ entry.concurrentExceptions.forEach(ex => values.push(ex));
Review comment:
nit: `values =
values.concat(concurrentExceptions).map(markGlobalFailure)`
where
```
function markGlobalFailure(exception: ExceptionInfoInterface) {
if (exception.taskName == null) {
exception.taskName = '(global failure)';
}
return exception;
}
```
This is all subjective style, but I find it easier to read...
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.ts
##########
@@ -61,7 +67,22 @@ export class JobExceptionsComponent implements OnInit {
this.rootException = 'No Root Exception';
}
this.truncated = exceptionHistory.truncated;
- this.listOfException = exceptionHistory.entries;
+ this.listOfException = exceptionHistory.entries.map(entry => {
+ const values: ExceptionInfoInterface[] = [];
+ values.push(entry);
Review comment:
I think this will add all concurrent exceptions to this entry as well,
which will be unnecessary clutter (have yet to find out how I can produce nice
exceptions to test...)
We can use something like this to extract the main exception:
```
function extractMainException(rootException: RootExceptionInfoInterface) {
const {concurrentExceptions, ...mainException} = rootException;
return mainException;
}
```
I first wanted to use lodash fp `omit`, but it only gives `Partial` return
types, which is annoying
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.ts
##########
@@ -22,6 +22,12 @@ import { ExceptionInfoInterface } from 'interfaces';
import { distinctUntilChanged, flatMap, tap } from 'rxjs/operators';
import { JobService } from 'services';
+export interface ExceptionInfoInterfaceWrapper {
+ selected: ExceptionInfoInterface;
+ values: ExceptionInfoInterface[];
Review comment:
nit/suggestion:
```suggestion
exceptions: ExceptionInfoInterface[];
```
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.ts
##########
@@ -30,7 +36,7 @@ import { JobService } from 'services';
})
export class JobExceptionsComponent implements OnInit {
rootException = '';
- listOfException: ExceptionInfoInterface[] = [];
+ listOfException: ExceptionInfoInterfaceWrapper[] = [];
Review comment:
```suggestion
listOfExceptions: ExceptionInfoInterfaceWrapper[] = [];
```
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.ts
##########
@@ -22,6 +22,12 @@ import { ExceptionInfoInterface } from 'interfaces';
import { distinctUntilChanged, flatMap, tap } from 'rxjs/operators';
import { JobService } from 'services';
+export interface ExceptionInfoInterfaceWrapper {
+ selected: ExceptionInfoInterface;
+ values: ExceptionInfoInterface[];
Review comment:
(renaming here would also require renaming some of the variables in the
html template)
##########
File path:
flink-runtime-web/web-dashboard/src/app/pages/job/exceptions/job-exceptions.component.html
##########
@@ -40,23 +40,25 @@
<ng-container *ngFor="let exception of
listOfException;trackBy:trackExceptionBy;">
<tr>
<td nzShowExpand [(nzExpand)]="exception.expand"></td>
- <td>{{exception.timestamp | date:'yyyy-MM-dd HH:mm:ss'}}</td>
- <td><div class="name">{{exception.exceptionName}}</div></td>
+ <td>{{exception.selected.timestamp | date:'yyyy-MM-dd
HH:mm:ss'}}</td>
+ <td><div
class="name">{{exception.selected.exceptionName}}</div></td>
<td>
- <div class="name">
- {{exception.taskName || "(global failure)"}}
- </div>
+ <nz-select style="width: 300px;"
[nzDisabled]="exception.values.length == 1" nzShowSearch
[(ngModel)]="exception.selected">
Review comment:
I am a bit skeptical of using `nzDisabled` in the case of a single
exception, but I will have to try and see if I have another suggestion.
--
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]
