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.

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

Custom config handler #6508 #6577

Merged
merged 1 commit into from
Nov 7, 2021
Merged

Custom config handler #6508 #6577

merged 1 commit into from
Nov 7, 2021

Conversation

ccoffline
Copy link
Contributor

Proposed changes

Support custom config handler callback and types.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation Update (if none of the other choices apply)
  • Code refactor (Modify the code structure, format the code, etc...)
  • Optimization. Including functional usability improvements and performance improvements.
  • Dependency. Such as changes related to third-party components.
  • Other.

Checklist

@morningman
Copy link
Contributor

Could your explain more about query traffic limit in your issue #6508 ?

@morningman morningman added area/config Issues or PRs related to configuration kind/improvement labels Sep 7, 2021
@ccoffline
Copy link
Contributor Author

Could your explain more about query traffic limit in your issue #6508 ?

I didn't put query traffic limit in this pr. This is still experimental on one of our clusters.

Some users want to limit query more flexibly than maxQueryInstances #6159. For example, all query should be rejected when queryInstances of this user reach 30000, and big queries that queryInstances more than 1000 should be rejected when queryInstances of this user reach 20000. Limiting rules like this will be complex and need to be modified at any time.

So we add a config that support this feature, using SpEL to evaluate whether to limit a query or not. The limiting expressions will be reset at any time, and the Expression needed to be compile as soon as setting the config. This is what this pr for, I think more configs will need callback in the future.

We are using 0.13 and #6159 is not supported yet. In master, there is a similar feature sql block rule #6192 which can support query traffic limit, maybe add a limit property. So I won't push our traffic limit pr to master, but just add config handler to support config setting callback.

@ccoffline
Copy link
Contributor Author

I just looked into #6192 and sql block rule cannot support traffic limit, but I think traffic limit is better set in the user properties.

Copy link
Contributor

@morningman morningman left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions github-actions bot added the approved Indicates a PR has been approved by one committer. label Nov 5, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2021

PR approved by at least one committer and no changes requested.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2021

PR approved by anyone and no changes requested.

@morningman morningman merged commit 3dd5570 into apache:master Nov 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by one committer. area/config Issues or PRs related to configuration kind/improvement reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Custom config handler, support callback when set mutable config at FE and BE
2 participants