Skip to content

Web console: add append to existing callout#13130

Merged
vogievetsky merged 1 commit intoapache:masterfrom
vogievetsky:fix_append_to_existing
Sep 22, 2022
Merged

Web console: add append to existing callout#13130
vogievetsky merged 1 commit intoapache:masterfrom
vogievetsky:fix_append_to_existing

Conversation

@vogievetsky
Copy link
Copy Markdown
Contributor

Add an issue callout in the data loader to notify the user that they are in an invalid spec state: appendToExisting: true + partitionSpec.type != 'dynamic' and give them a CTA on all the relevant screens to help fix it.

image

Full screen:

image

onChangeSpec(newSpec: Partial<IngestionSpec>): void;
}

export const AppendToExistingIssue = React.memo(function AppendToExistingIssue(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Do we want to call it AppendToExistingIssue?
To me, issue implies that there is a known bug and we are yet to fix it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Within the context of the console that is exactly what it is... the user has selected UI options that put the spec in the data loader into a state that can not be submitted. And this callout prompts the user to fix it. Note that I am not making a judgment on whether this behavior is an "issue in Druid" just that the data loader has an internal issue.

The term "issue" is used in the same context all over the console codebase, for example https://github.com/apache/druid/blob/master/web-console/src/druid-models/ingestion-spec/ingestion-spec.tsx#L240 - this function will flag an "issue" if spec.dataSchema.dataSource is not set.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I see. Thanks for the clarification 👍🏻

Copy link
Copy Markdown
Contributor

@kfaraz kfaraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@vogievetsky vogievetsky merged commit f1d3728 into apache:master Sep 22, 2022
@vogievetsky vogievetsky deleted the fix_append_to_existing branch September 22, 2022 02:39
vogievetsky added a commit to vogievetsky/druid that referenced this pull request Sep 26, 2022
@imply-cheddar
Copy link
Copy Markdown
Contributor

I would like to request a change in the suggested CTA to ask them to remove the appendToExisting instead of push people to dynamic partitioning. Dynamic partitioning when mixed with batch ingestion leads to bad clusters more often than not, we should guide people towards things that will leave them in a good state instead.

abhishekagarwal87 pushed a commit that referenced this pull request Sep 28, 2022
* fix number of expected functions (#13050)

* default to no compare (#13041)

* quote columns, datasources in auto complete if needed (#13060)

* Web console: better detection for arrays containing objects (#13077)

* better detection for arrays containing objects

* include boolean also

* link to error docs (#13094)

* Web console: correctly escape path based flatten specs (#13105)

* fix path generation

* do escape

* fix replace

* fix replace for good

* append to exisitng callout (#13130)

* better spec conversion with issues (#13136)

* bump version to 24.0.1
@abhishekagarwal87 abhishekagarwal87 added this to the 24.0.1 milestone Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants