You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2022/09/20 17:57:46 UTC
[GitHub] [druid] vogievetsky opened a new pull request, #13130: Web console: add append to existing callout
vogievetsky opened a new pull request, #13130:
URL: https://github.com/apache/druid/pull/13130
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.
<img width="316" alt="image" src="https://user-images.githubusercontent.com/177816/191330138-3813d1c8-6d76-4212-9cd2-ad675791f233.png">
Full screen:
<img width="1300" alt="image" src="https://user-images.githubusercontent.com/177816/191330287-d668b828-2374-4185-b0a4-982b9ba9c435.png">
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13130: Web console: add append to existing callout
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13130:
URL: https://github.com/apache/druid/pull/13130#discussion_r976002950
##########
web-console/src/views/load-data-view/info-messages.tsx:
##########
@@ -216,3 +217,48 @@ export const SpecMessage = React.memo(function SpecMessage() {
</FormGroup>
);
});
+
+export interface AppendToExistingIssueProps {
+ spec: Partial<IngestionSpec>;
+ onChangeSpec(newSpec: Partial<IngestionSpec>): void;
+}
+
+export const AppendToExistingIssue = React.memo(function AppendToExistingIssue(
Review Comment:
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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky merged pull request #13130: Web console: add append to existing callout
Posted by GitBox <gi...@apache.org>.
vogievetsky merged PR #13130:
URL: https://github.com/apache/druid/pull/13130
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] vogievetsky commented on a diff in pull request #13130: Web console: add append to existing callout
Posted by GitBox <gi...@apache.org>.
vogievetsky commented on code in PR #13130:
URL: https://github.com/apache/druid/pull/13130#discussion_r976061312
##########
web-console/src/views/load-data-view/info-messages.tsx:
##########
@@ -216,3 +217,48 @@ export const SpecMessage = React.memo(function SpecMessage() {
</FormGroup>
);
});
+
+export interface AppendToExistingIssueProps {
+ spec: Partial<IngestionSpec>;
+ onChangeSpec(newSpec: Partial<IngestionSpec>): void;
+}
+
+export const AppendToExistingIssue = React.memo(function AppendToExistingIssue(
Review Comment:
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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] kfaraz commented on a diff in pull request #13130: Web console: add append to existing callout
Posted by GitBox <gi...@apache.org>.
kfaraz commented on code in PR #13130:
URL: https://github.com/apache/druid/pull/13130#discussion_r976170890
##########
web-console/src/views/load-data-view/info-messages.tsx:
##########
@@ -216,3 +217,48 @@ export const SpecMessage = React.memo(function SpecMessage() {
</FormGroup>
);
});
+
+export interface AppendToExistingIssueProps {
+ spec: Partial<IngestionSpec>;
+ onChangeSpec(newSpec: Partial<IngestionSpec>): void;
+}
+
+export const AppendToExistingIssue = React.memo(function AppendToExistingIssue(
Review Comment:
Hmm, I see. Thanks for the clarification 👍🏻
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
[GitHub] [druid] imply-cheddar commented on pull request #13130: Web console: add append to existing callout
Posted by GitBox <gi...@apache.org>.
imply-cheddar commented on PR #13130:
URL: https://github.com/apache/druid/pull/13130#issuecomment-1258947077
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.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org