Airblader commented on a change in pull request #16166:
URL: https://github.com/apache/flink/pull/16166#discussion_r652361282
##########
File path: flink-runtime-web/web-dashboard/src/app/app.component.html
##########
@@ -49,6 +49,9 @@ <h1>Apache Flink Dashboard</h1></a>
<span><i nz-icon type="upload"></i><span>Submit New Job</span></span>
</li>
</ul>
+ <nz-affix *ngIf="!collapsed" style="position: absolute; bottom: 15px;
left:15px" id="affix-container-target">
Review comment:
Inline styling is a bad practice, the minimum should be to use a CSS
class instead and move the styles into `app.component.less`.
Absolute positioning is kind of an art form. It can be the fitting solution
in some cases, but most of the time it's an escape hatch that just causes
problems (for example when collapsing the menu, as you noticed), because it
takes the element entirely out of the document flow. If the intent is simply to
have this on the bottom of the menu, an easier solution would be
```
margin: auto 15px 15px 15px;
```
I haven't verified this here, but it should work (we use the same thing in
Ververica Platform).
(The final nit would be that I'm guessing the value of 15px was more or less
chosen at random, AntD actually uses a different progression for spacing, so
technically 12px or 16px would be in line with the design language.)
##########
File path: flink-runtime-web/web-dashboard/src/app/app.module.ts
##########
@@ -83,7 +84,7 @@ export function AppInitServiceFactory(
@NgModule({
declarations: [AppComponent],
- imports: [BrowserModule, AppRoutingModule, NgZorroAntdModule, FormsModule,
HttpClientModule, BrowserAnimationsModule],
+ imports: [BrowserModule, AppRoutingModule, NgZorroAntdModule, FormsModule,
HttpClientModule, BrowserAnimationsModule, ShareModule],
Review comment:
This seems to be improperly formatted, both the import and this line
here. If the tooling doesn't complain then I guess we have no tooling for the
UI for this, in which case feel free to disregard.
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.html
##########
@@ -0,0 +1,27 @@
+<!--
+ ~ 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.
+ -->
+
+<button nz-button nzType="primary" (click)="requestArchive()">Request New Log
Archive</button>
+<button nz-button nzType="primary" [hidden]="hideDownloadButton"
(click)="downloadArchive()" style="margin-top:5px">Download Log Archive</button>
Review comment:
Don't use inline CSS for the styling (and use 4px?), also in the lines
below.
##########
File path:
flink-runtime-web/web-dashboard/src/app/services/logs-bundler.service.ts
##########
@@ -0,0 +1,49 @@
+/*
+ * 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 { HttpClient } from '@angular/common/http';
+import { Injectable } from '@angular/core';
+import { EMPTY } from 'rxjs';
+import { catchError } from 'rxjs/operators';
+import { BASE_URL } from '../app.config';
+import {LogsBundlerStatus} from "../interfaces/logs-bundler";
+
+@Injectable({
+ providedIn: 'root'
+})
+export class LogsBundlerService {
+ constructor(private httpClient: HttpClient) {}
+
+ /**
+ * trigger log bundling
+ */
+ triggerBundle() {
+ var res =
this.httpClient.get(`${BASE_URL}/logbundler?action=trigger`).pipe(catchError(()
=> {
+ return EMPTY
+ }));
+ // TODO this seems to be needed to trigger the call?
+ res.toPromise().then(() => {
+ console.log("fulfilled")
+ })
+ return res;
+ }
Review comment:
```suggestion
triggerBundle() {
return
this.httpClient.get(`${BASE_URL}/logbundler?action=trigger`).pipe(
catchError(() => EMPTY)
);
}
```
The `subscribe` should not actually happen here, but rather at the caller
site anyway. Otherwise this is very surprising behavior. Either you
fire-and-forget, meaning you don't return anything, or you return the
observable and let the caller manage the subscription. Since this is a service
which acts as a facade to the API, I would always opt for returning the
observable either way so the caller can choose whether and what to do with the
response.
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
+ @Input() statusObservable: Observable<LogsBundlerStatus>;
+ hideSpinner: boolean = true;
+ message: string = ""
+ hideDownloadButton: boolean = true;
+
+ constructor(private logBundlerService: LogsBundlerService,
+ private statusService: StatusService,
+ private cdr: ChangeDetectorRef) {
+ }
+
+ ngOnInit() {
+ this.statusObservable = this.statusService.refresh$
+ .pipe(
+ takeUntil(this.destroy$),
+ flatMap(() =>
+ this.logBundlerService.getStatus()
+ )
+ )
+ this.statusObservable.subscribe( status => {
+ this.message = status.message;
+ this.hideSpinner = true;
+ this.hideDownloadButton = true;
+
+ if(status.status == "PROCESSING") {
Review comment:
In JavaScript, using `==` is a deadly sin these days, and we use `===`
instead. :-)
##########
File path:
flink-runtime-web/web-dashboard/src/app/services/logs-bundler.service.ts
##########
@@ -0,0 +1,49 @@
+/*
+ * 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 { HttpClient } from '@angular/common/http';
+import { Injectable } from '@angular/core';
+import { EMPTY } from 'rxjs';
+import { catchError } from 'rxjs/operators';
+import { BASE_URL } from '../app.config';
+import {LogsBundlerStatus} from "../interfaces/logs-bundler";
+
+@Injectable({
+ providedIn: 'root'
+})
+export class LogsBundlerService {
+ constructor(private httpClient: HttpClient) {}
+
+ /**
+ * trigger log bundling
+ */
+ triggerBundle() {
+ var res =
this.httpClient.get(`${BASE_URL}/logbundler?action=trigger`).pipe(catchError(()
=> {
+ return EMPTY
Review comment:
Mapping errors to `EMPTY` means this observable will emit nothing (and
just complete) in a failure case. I'll have to read on to see if this is
problematic.
Edit from my future self: It means that if the API starts failing, the UI
will simply not update anymore. Probably not what we'd want, so a nicer
solution might be to map to a "fake" response, or let the error propagate and
handle it at the caller site.
This is better than doing nothing at all, at least, since it prevents the
error from clogging up the console. But it would leave the user in the dark.
I'll leave it to you whether this is something we want to address or not given
that this is probably the status quo of how things are done in Flink UI and
thus this doesn't make it worse.
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
+ @Input() statusObservable: Observable<LogsBundlerStatus>;
+ hideSpinner: boolean = true;
+ message: string = ""
+ hideDownloadButton: boolean = true;
+
+ constructor(private logBundlerService: LogsBundlerService,
+ private statusService: StatusService,
+ private cdr: ChangeDetectorRef) {
+ }
+
+ ngOnInit() {
+ this.statusObservable = this.statusService.refresh$
Review comment:
There's also a whole thing about whether this should use `flatMap`,
`concatMap` or `switchMap`, as all of these have slightly different semantics.
With `mergeMap`, if requests are too slow, we'll end up running them in
parallel and stitching them together in whichever order they finish, including
running out of order. With `switchMap` we avoid this, but it could insted lead
to it never emitting at all. As this is an unlikely scenario it doesn't matter,
but just pointing out the considerations that typically would go into the
choice of operator.
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
+ @Input() statusObservable: Observable<LogsBundlerStatus>;
+ hideSpinner: boolean = true;
+ message: string = ""
+ hideDownloadButton: boolean = true;
+
+ constructor(private logBundlerService: LogsBundlerService,
+ private statusService: StatusService,
+ private cdr: ChangeDetectorRef) {
+ }
+
+ ngOnInit() {
+ this.statusObservable = this.statusService.refresh$
+ .pipe(
+ takeUntil(this.destroy$),
+ flatMap(() =>
+ this.logBundlerService.getStatus()
+ )
Review comment:
My brain has trouble figuring out which opening parenthesis this closing
one belongs to. :-) Is the formatting broken here?
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
Review comment:
```suggestion
private destroy$ = new Subject<void>();
```
(If you want to be more precise, it can even be `private readonly`, but this
isn't used anywhere else either)
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
+ @Input() statusObservable: Observable<LogsBundlerStatus>;
+ hideSpinner: boolean = true;
+ message: string = ""
+ hideDownloadButton: boolean = true;
+
+ constructor(private logBundlerService: LogsBundlerService,
+ private statusService: StatusService,
+ private cdr: ChangeDetectorRef) {
+ }
+
+ ngOnInit() {
+ this.statusObservable = this.statusService.refresh$
+ .pipe(
+ takeUntil(this.destroy$),
Review comment:
The `takeUntil` should go after `flatMap`, otherwise this creates a
subscription leak (which isn't too bad here since it's just one-off HTTP
requests, but still).
As a rule of thumb, `takeUntil(destroy$)` should generally be the last
operator except for a few select scenarios.
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
+ @Input() statusObservable: Observable<LogsBundlerStatus>;
Review comment:
It can either be an input, or you assemble it in `ngOnInit`, but both is
odd (and passing observables as inputs is a dangerous game anyway). Given that
you aren't actually using this input, remove `@Input()`.
Naming-wise the idiomatic convention is a `$` suffix, i.e. `status$`, rather
than `xObservable`. I won't bike-shed here, so feel free to disregard.
This and all other properties below can also be made `private`.
##########
File path: flink-runtime-web/web-dashboard/src/app/interfaces/logs-bundler.ts
##########
@@ -0,0 +1,22 @@
+/*
+ * 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.
+ */
+
+export interface LogsBundlerStatus {
+ status: string,
Review comment:
These lines should both end with `;`.
##########
File path: flink-runtime-web/web-dashboard/src/app/interfaces/logs-bundler.ts
##########
@@ -0,0 +1,22 @@
+/*
+ * 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.
+ */
+
+export interface LogsBundlerStatus {
+ status: string,
Review comment:
I'm assuming the possible values for `status` are known, so we could be
more precise than `string` here:
```
status: "PROCESSING" | "BUNDLE_READY" | "… possible other ones…";
```
With string literal types we get completion and better type-safety for where
you compare the status against these values.
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
+ @Input() statusObservable: Observable<LogsBundlerStatus>;
+ hideSpinner: boolean = true;
+ message: string = ""
+ hideDownloadButton: boolean = true;
+
+ constructor(private logBundlerService: LogsBundlerService,
+ private statusService: StatusService,
+ private cdr: ChangeDetectorRef) {
+ }
+
+ ngOnInit() {
+ this.statusObservable = this.statusService.refresh$
+ .pipe(
+ takeUntil(this.destroy$),
+ flatMap(() =>
+ this.logBundlerService.getStatus()
+ )
+ )
+ this.statusObservable.subscribe( status => {
+ this.message = status.message;
+ this.hideSpinner = true;
+ this.hideDownloadButton = true;
+
+ if(status.status == "PROCESSING") {
+ this.hideSpinner = false;
+ }
+ if (status.status == "BUNDLE_READY") {
+ this.hideDownloadButton = false;
+ }
+ this.cdr.markForCheck();
+ })
+ }
+
+ requestArchive() {
+ this.logBundlerService.triggerBundle();
+ this.hideSpinner = false;
+ this.hideDownloadButton = true;
+ }
+ downloadArchive() {
Review comment:
Personally, I think this method belongs into the service since it builds
the specific path of the API, which is generally one of the things you want to
encapsulate in those services so that components don't need to be aware of the
API.
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
+ @Input() statusObservable: Observable<LogsBundlerStatus>;
+ hideSpinner: boolean = true;
+ message: string = ""
+ hideDownloadButton: boolean = true;
+
+ constructor(private logBundlerService: LogsBundlerService,
+ private statusService: StatusService,
+ private cdr: ChangeDetectorRef) {
+ }
+
+ ngOnInit() {
+ this.statusObservable = this.statusService.refresh$
+ .pipe(
+ takeUntil(this.destroy$),
+ flatMap(() =>
+ this.logBundlerService.getStatus()
+ )
+ )
+ this.statusObservable.subscribe( status => {
+ this.message = status.message;
+ this.hideSpinner = true;
+ this.hideDownloadButton = true;
+
+ if(status.status == "PROCESSING") {
+ this.hideSpinner = false;
+ }
+ if (status.status == "BUNDLE_READY") {
+ this.hideDownloadButton = false;
+ }
Review comment:
```suggestion
this.hideSpinner = (status.status !== "PROCESSING");
this.hideDownloadButton = (status.status !== "BUNDLE_READY");
```
?
##########
File path:
flink-runtime-web/web-dashboard/src/app/share/customize/logs-bundler/logs-bundler.component.ts
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 {ChangeDetectionStrategy, ChangeDetectorRef, Component, Input, OnInit}
from '@angular/core';
+import {flatMap, takeUntil} from "rxjs/operators";
+import { Observable, Subject} from "rxjs";
+import { StatusService} from "services";
+import {LogsBundlerService} from "../../../services/logs-bundler.service";
+import {BASE_URL} from "config";
+import {LogsBundlerStatus} from "../../../interfaces/logs-bundler";
+
+@Component({
+ selector: 'flink-logs-bundler',
+ templateUrl: './logs-bundler.component.html',
+ styleUrls: [],
+ changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class LogsBundlerComponent implements OnInit{
+ destroy$ = new Subject();
+ @Input() statusObservable: Observable<LogsBundlerStatus>;
+ hideSpinner: boolean = true;
+ message: string = ""
+ hideDownloadButton: boolean = true;
+
+ constructor(private logBundlerService: LogsBundlerService,
+ private statusService: StatusService,
+ private cdr: ChangeDetectorRef) {
+ }
+
+ ngOnInit() {
+ this.statusObservable = this.statusService.refresh$
Review comment:
There's actually no reason to even have `statusObservable` as a member
field at all. You can just use a local variable.
```
const status$ = this.statusService.…;
status$.subscribe(…);
```
Since nothing much happens here with that variable you can even just combine
it altogether:
```
this.statusService.refresh$.pipe(
flatMap(() => this.logBundlerService.getStatus()),
takeUntil(this.destroy$),
).subscribe(status => {
// …
});
```
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
