Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make planner errors be user persona #17437

Merged
merged 3 commits into from
Nov 4, 2024
Merged

make planner errors be user persona #17437

merged 3 commits into from
Nov 4, 2024

Conversation

TSFenwick
Copy link
Contributor

@TSFenwick TSFenwick commented Oct 30, 2024 •
edited
Loading

Description

Change the persona for errors within the planner from Admin to User. The ADMIN persona is meant to be "a persona who is interacting with admin APIs and understands Druid query concepts". This isn't an admin API, it's a query API. L:ow quality error messages being returned to the correct audience is better than hiding all error messages.

The errors that can be returned back can be user solvable, and other times requires a druid expert. But the errors do not leak information that should only be seen by more expert/privileged personas.

The original ADMIN persona showed some reticence to tag low-quality error messages with a USER persona. but it really does seem user-directed to me so USER to me would make sense.

Release note

Key changed/added classes in this PR
  • sql/src/main/java/org/apache/druid/sql/calcite/planner/QueryHandler.java

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • a release note entry in the PR description.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@TSFenwick
make planner errors be user persona.
e914c17 
planner errors while might be above a user level, do not leak information that should
be only given to admins. The occasional planner errors that are user level are worthwhile
sharing with the user over giving them information they might not be able to resolve
@TSFenwick TSFenwick marked this pull request as ready for review October 30, 2024 19:48
@github-actions github-actions bot added Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 labels Oct 30, 2024
@TSFenwick TSFenwick changed the title make planner errors be user persona. make planner errors be user persona Oct 31, 2024
abhishekrb19
abhishekrb19 approved these changes Oct 31, 2024
View reviewed changes
Copy link
Contributor

@abhishekrb19 abhishekrb19 left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me. If users see low-quality planning errors (alongside useful ones) as a result of this change, it provides an opportunity for us to improve those low-quality ones over time.

@abhishekrb19 abhishekrb19 merged commit e4cdbca into apache:master Nov 4, 2024
89 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhishekrb19 abhishekrb19 abhishekrb19 approved these changes

Assignees
No one assigned
Labels
Area - Batch Ingestion Area - MSQ For multi stage queries - https://github.com/apache/druid/issues/12262 Area - Querying
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@TSFenwick @abhishekrb19