You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@nifi.apache.org by "mcgilman (via GitHub)" <gi...@apache.org> on 2024/02/09 22:04:44 UTC
[PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
mcgilman opened a new pull request, #8386:
URL: https://github.com/apache/nifi/pull/8386
NIFI-12767:
- Error handling in Provenance and Lineage.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
Posted by "mcgilman (via GitHub)" <gi...@apache.org>.
mcgilman commented on code in PR #8386:
URL: https://github.com/apache/nifi/pull/8386#discussion_r1488532914
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/provenance-event-listing/provenance-event-listing.effects.ts:
##########
@@ -311,18 +321,31 @@ export class ProvenanceEventListingEffects {
dialogReference.componentInstance.replay
.pipe(takeUntil(dialogReference.afterClosed()))
.subscribe(() => {
- this.provenanceService.replay(request.id).subscribe(() => {
- this.store.dispatch(
- ProvenanceEventListingActions.showOkDialog({
- title: 'Provenance',
- message: 'Successfully submitted replay request.'
- })
- );
+ this.provenanceService.replay(request.id).subscribe({
+ next: () => {
+ dialogReference.close();
+
+ this.store.dispatch(
+ ProvenanceEventListingActions.showOkDialog({
+ title: 'Provenance',
+ message: 'Successfully submitted replay request.'
+ })
+ );
+ },
+ error: (errorResponse: HttpErrorResponse) => {
+ this.store.dispatch(
+ ErrorActions.snackBarError({ error: errorResponse.error })
+ );
Review Comment:
I will also update to close the dialog so it error is shown more clearly.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
Posted by "rfellows (via GitHub)" <gi...@apache.org>.
rfellows merged PR #8386:
URL: https://github.com/apache/nifi/pull/8386
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
Posted by "mcgilman (via GitHub)" <gi...@apache.org>.
mcgilman commented on code in PR #8386:
URL: https://github.com/apache/nifi/pull/8386#discussion_r1488490343
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/provenance-event-listing/provenance-event-listing.effects.ts:
##########
@@ -311,18 +321,31 @@ export class ProvenanceEventListingEffects {
dialogReference.componentInstance.replay
.pipe(takeUntil(dialogReference.afterClosed()))
.subscribe(() => {
- this.provenanceService.replay(request.id).subscribe(() => {
- this.store.dispatch(
- ProvenanceEventListingActions.showOkDialog({
- title: 'Provenance',
- message: 'Successfully submitted replay request.'
- })
- );
+ this.provenanceService.replay(request.id).subscribe({
+ next: () => {
+ dialogReference.close();
+
+ this.store.dispatch(
+ ProvenanceEventListingActions.showOkDialog({
+ title: 'Provenance',
+ message: 'Successfully submitted replay request.'
+ })
+ );
+ },
+ error: (errorResponse: HttpErrorResponse) => {
+ this.store.dispatch(
+ ErrorActions.snackBarError({ error: errorResponse.error })
+ );
Review Comment:
I made this choice because the error isn't based on user input (there's nothing they can change and resubmit) in the dialog and a snackbar error was used in similar situations like when loading descriptors for new services and parameters.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
Posted by "mcgilman (via GitHub)" <gi...@apache.org>.
mcgilman commented on code in PR #8386:
URL: https://github.com/apache/nifi/pull/8386#discussion_r1488499673
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/queue/state/queue-listing/index.ts:
##########
@@ -102,7 +102,7 @@ export interface FlowFileDialogRequest {
export interface QueueListingState {
activeListingRequest: ListingRequest | null;
- completedListingRequest: ListingRequest | null;
+ completedListingRequest: ListingRequest;
Review Comment:
Let me spend some time re-evaluating. Rather than being null, I provided an empty state in the provenance and lineage reducers similar to how the canvas state is initialized in the flow reducer. I should be able to make this change but may need further adjustments where this state is consumed.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
Posted by "rfellows (via GitHub)" <gi...@apache.org>.
rfellows commented on code in PR #8386:
URL: https://github.com/apache/nifi/pull/8386#discussion_r1488335062
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/queue/state/queue-listing/index.ts:
##########
@@ -102,7 +102,7 @@ export interface FlowFileDialogRequest {
export interface QueueListingState {
activeListingRequest: ListingRequest | null;
- completedListingRequest: ListingRequest | null;
+ completedListingRequest: ListingRequest;
Review Comment:
It seems peculiar that the `completedListingRequest` would never be null. I would assume it would remain null until there is a request that has completed.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/ui/provenance-event-listing/provenance-event-table/provenance-event-table.component.spec.ts:
##########
@@ -20,14 +20,29 @@ import { ComponentFixture, TestBed } from '@angular/core/testing';
import { ProvenanceEventTable } from './provenance-event-table.component';
import { MatTableModule } from '@angular/material/table';
import { BrowserAnimationsModule } from '@angular/platform-browser/animations';
+import { provideMockStore } from '@ngrx/store/testing';
+import { initialState } from '../../../../../state/error/error.reducer';
+import { Component } from '@angular/core';
describe('ProvenanceEventTable', () => {
let component: ProvenanceEventTable;
let fixture: ComponentFixture<ProvenanceEventTable>;
+ @Component({
+ selector: 'error-banner',
+ standalone: true,
+ template: ''
+ })
+ class MockErrorBanner {}
+
beforeEach(() => {
TestBed.configureTestingModule({
- imports: [ProvenanceEventTable, MatTableModule, BrowserAnimationsModule]
+ imports: [ProvenanceEventTable, MockErrorBanner, MatTableModule, BrowserAnimationsModule],
Review Comment:
In general, we should be using `NoopAnimationsModule` rather than `BrowserAnimationsModule` in tests to make them as efficient as possible.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/lineage/index.ts:
##########
@@ -66,7 +66,7 @@ export interface Lineage {
}
export interface LineageState {
- lineage: Lineage | null;
- error: string | null;
+ activeLineage: Lineage | null;
+ completedLineage: Lineage;
status: 'pending' | 'loading' | 'error' | 'success';
Review Comment:
Can we remove `error`?
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/ui/common/provenance-event-dialog/provenance-event-dialog.component.html:
##########
@@ -17,6 +17,7 @@
<h2 mat-dialog-title>Provenance Event</h2>
<div class="provenance-event">
+ <error-banner></error-banner>
Review Comment:
We might need to devise some way of giving these banners some context. I found a scenario where I had a banner error while on the lineage screen then opened the View Details option of one of the nodes and now the error is in 2 places at the same time. Dismissing one, dismisses them both.
<img width="1729" alt="Screenshot 2024-02-13 at 1 43 47 PM" src="https://github.com/apache/nifi/assets/713866/edd94d1f-d187-4a7d-900b-b32ee7fac53d">
Maybe we need to specify the source of the banner error in the store (optionally???) when firing them, and then the banner elements can take an input of the context they should be interested in?
```
<error-banner context="provenance-event-diaglog" ></error-banner>
```
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/ui/provenance-event-listing/provenance-event-table/provenance-event-table.component.html:
##########
@@ -15,7 +15,8 @@
~ limitations under the License.
-->
-<div class="provenance-event-table h-full">
+<div class="provenance-event-table h-full flex flex-col gap-y-2">
+ <error-banner></error-banner>
Review Comment:
As this banner is used for both provenance and lineage issues, we might need to clear them when we switch from one view to the other.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/provenance-event-listing/provenance-event-listing.effects.ts:
##########
@@ -205,18 +211,26 @@ export class ProvenanceEventListingEffects {
)
);
- deleteProvenanceQuery$ = createEffect(
- () =>
- this.actions$.pipe(
- ofType(ProvenanceEventListingActions.deleteProvenanceQuery),
- concatLatestFrom(() => [this.store.select(selectProvenanceId), this.store.select(selectClusterNodeId)]),
- tap(([, id, clusterNodeId]) => {
- if (id) {
- this.provenanceService.deleteProvenanceQuery(id, clusterNodeId).subscribe();
- }
- })
- ),
- { dispatch: false }
+ deleteProvenanceQuery$ = createEffect(() =>
+ this.actions$.pipe(
+ ofType(ProvenanceEventListingActions.deleteProvenanceQuery),
+ concatLatestFrom(() => [
+ this.store.select(selectActiveProvenanceId),
+ this.store.select(selectClusterNodeId)
+ ]),
+ tap(([, id, clusterNodeId]) => {
+ this.dialog.closeAll();
+
+ if (id) {
+ this.provenanceService.deleteProvenanceQuery(id, clusterNodeId).subscribe({
+ error: (errorResponse: HttpErrorResponse) => {
+ this.store.dispatch(ErrorActions.snackBarError({ error: errorResponse.error }));
+ }
+ });
+ }
Review Comment:
Handling of the error is fine. However, what are your thoughts on not showing the error to the user in this scenario? They can't do anything about it and is pretty meaningless to them as they did get their results. It might just be more confusing to show this error.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/provenance-event-listing/provenance-event-listing.effects.ts:
##########
@@ -58,10 +63,14 @@ export class ProvenanceEventListingEffects {
response
})
),
- catchError((error) =>
+ catchError(() =>
of(
- ProvenanceEventListingActions.provenanceApiError({
- error: error.error
+ ProvenanceEventListingActions.loadProvenanceOptionsSuccess({
+ response: {
+ provenanceOptions: {
+ searchableFields: []
+ }
+ }
Review Comment:
If the call to `getSearchOptions` fails, we end up with a partially complete search dialog where only the date fields are available and the buttons aren't at the bottom.
<img width="774" alt="Screenshot 2024-02-13 at 1 03 18 PM" src="https://github.com/apache/nifi/assets/713866/fec72bea-8abb-4572-b859-efd7c70aad64">
I'm not certain on what to do in this case. Does a banner error make sense? Should we just close the dialog and throw up a snackbar error?
If we can;t do anything with the dialog in the state it is in, we should probably close it and go with a snackbar error. However, It seems that you can still submit a valid query from it. So, maybe at banner error is better. We should also be sure the buttons are snapped to the bottom too.
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/provenance-event-listing/provenance-event-listing.effects.ts:
##########
@@ -311,18 +321,31 @@ export class ProvenanceEventListingEffects {
dialogReference.componentInstance.replay
.pipe(takeUntil(dialogReference.afterClosed()))
.subscribe(() => {
- this.provenanceService.replay(request.id).subscribe(() => {
- this.store.dispatch(
- ProvenanceEventListingActions.showOkDialog({
- title: 'Provenance',
- message: 'Successfully submitted replay request.'
- })
- );
+ this.provenanceService.replay(request.id).subscribe({
+ next: () => {
+ dialogReference.close();
+
+ this.store.dispatch(
+ ProvenanceEventListingActions.showOkDialog({
+ title: 'Provenance',
+ message: 'Successfully submitted replay request.'
+ })
+ );
+ },
+ error: (errorResponse: HttpErrorResponse) => {
+ this.store.dispatch(
+ ErrorActions.snackBarError({ error: errorResponse.error })
+ );
Review Comment:
A snackbar in this case is a bit odd. The dialog is open and the snackbar pops up well under the dialog. almost making it seem unrelated to the action they just attempted (replay of the event). Maybe a banner here?
<img width="795" alt="Screenshot 2024-02-13 at 1 31 41 PM" src="https://github.com/apache/nifi/assets/713866/795d2b2f-f389-4872-a596-3794fe3b9f73">
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/lineage/index.ts:
##########
@@ -66,7 +66,7 @@ export interface Lineage {
}
export interface LineageState {
- lineage: Lineage | null;
- error: string | null;
+ activeLineage: Lineage | null;
+ completedLineage: Lineage;
Review Comment:
Why not support `null` for `completedLineage`?
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
Posted by "mcgilman (via GitHub)" <gi...@apache.org>.
mcgilman commented on code in PR #8386:
URL: https://github.com/apache/nifi/pull/8386#discussion_r1488649389
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/provenance-event-listing/provenance-event-listing.effects.ts:
##########
@@ -58,10 +63,14 @@ export class ProvenanceEventListingEffects {
response
})
),
- catchError((error) =>
+ catchError(() =>
of(
- ProvenanceEventListingActions.provenanceApiError({
- error: error.error
+ ProvenanceEventListingActions.loadProvenanceOptionsSuccess({
+ response: {
+ provenanceOptions: {
+ searchableFields: []
+ }
+ }
Review Comment:
This behavior was from the current NiFi UI. The searchable fields are not loaded when the dialog opens, but rather once when the page loads. I'll update the dialog to indicate when there are no searchable fields available.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
Posted by "mcgilman (via GitHub)" <gi...@apache.org>.
mcgilman commented on code in PR #8386:
URL: https://github.com/apache/nifi/pull/8386#discussion_r1488493933
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/ui/common/provenance-event-dialog/provenance-event-dialog.component.html:
##########
@@ -17,6 +17,7 @@
<h2 mat-dialog-title>Provenance Event</h2>
<div class="provenance-event">
+ <error-banner></error-banner>
Review Comment:
I meant to delete this `error-banner` because there was no scenario where errors could surface from the dialog directly. If I do, we could punt the contextual banner errors down the road until we hit it again.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] NIFI-12767: Error handling in Provenance and Lineage [nifi]
Posted by "mcgilman (via GitHub)" <gi...@apache.org>.
mcgilman commented on code in PR #8386:
URL: https://github.com/apache/nifi/pull/8386#discussion_r1488506126
##########
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-frontend/src/main/nifi/src/app/pages/provenance/state/lineage/index.ts:
##########
@@ -66,7 +66,7 @@ export interface Lineage {
}
export interface LineageState {
- lineage: Lineage | null;
- error: string | null;
+ activeLineage: Lineage | null;
+ completedLineage: Lineage;
status: 'pending' | 'loading' | 'error' | 'success';
Review Comment:
I think so. It was needed in an earlier iteration of this solution. I'll remove it here and in the provenance state.
--
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@nifi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org