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

[Feature] Make blacklist persistent #55014

Merged
merged 5 commits into from
Jan 16, 2025
Merged

[Feature] Make blacklist persistent #55014

merged 5 commits into from
Jan 16, 2025

Conversation

SHaaD94
Copy link
Contributor

@SHaaD94 SHaaD94 commented Jan 13, 2025 •
edited by murphyatwork
Loading

Why I'm doing:

In Memory Sql Blacklist is inconvenient to work with, since blacklist entries must be added again after any FE node restart.

What I'm doing:

Persistence is implemented as following:

  • Blacklist add/delete request go through edit log (journal)
  • Added storing/reloading metadata for blacklist
  • Requests for addition/deletion of sql blacklist will be forwarded to a leader

Fixes #55013

What type of PR is this:

  • BugFix
  • Feature
  • Enhancement
  • Refactor
  • UT
  • Doc
  • Tool

Does this PR entail a change in behavior?

  • Yes, this PR will result in a change in behavior.
  • No, this PR will not result in a change in behavior.

If yes, please specify the type of change:

  • Interface/UI changes: syntax, type conversion, expression evaluation, display information
  • Parameter changes: default values, similar parameters but with different default values
  • Policy changes: use new policy to replace old one, functionality automatically enabled
  • Feature removed
  • Miscellaneous: upgrade & downgrade compatibility, etc.

Checklist:

  • I have added test cases for my bug fix or my new feature
  • This pr needs user documentation (for new or modified features or behaviors)
    • I have added documentation for my new feature or new function
  • This is a backport pr

Bugfix cherry-pick branch check:

  • I have checked the version labels which the pr will be auto-backported to the target branch
    • 3.4
    • 3.3
    • 3.2
    • 3.1
    • 3.0

@SHaaD94 SHaaD94 requested a review from a team as a code owner January 13, 2025 08:37
@CLAassistant
Copy link

CLAassistant commented Jan 13, 2025 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@wanpengfei-git wanpengfei-git requested a review from a team January 13, 2025 08:37
@SHaaD94 SHaaD94 changed the title Make blacklist persistent [Feature] Make blacklist persistent Jan 13, 2025
@SHaaD94 SHaaD94 force-pushed the forked_origin_main branch 2 times, most recently from a4caad8 to 7c75cd3 Compare January 13, 2025 09:12
@SHaaD94
Make blacklist persistent
3a8e612 
Signed-off-by: Evgeniy Zuykin <[email protected]>
@SHaaD94 SHaaD94 force-pushed the forked_origin_main branch from 7c75cd3 to 3a8e612 Compare January 13, 2025 09:21
@murphyatwork
Copy link
Contributor

some advices:

  1. it will change the behavior, so need to check the box(we will mention it in the release note). as well it's better the change the document correspondingly.
  2. it's better to add some basic sql tests(test/sql/), which is easier to maintain and understand

@HangyuanLiu HangyuanLiu self-assigned this Jan 13, 2025
@gengjun-git gengjun-git self-assigned this Jan 13, 2025
@SHaaD94
Copy link
Contributor Author

SHaaD94 commented Jan 13, 2025 •
edited
Loading

some advices:

  1. it will change the behavior, so need to check the box(we will mention it in the release note). as well it's better the change the document correspondingly.
  2. it's better to add some basic sql tests(test/sql/), which is easier to maintain and understand

Thanks, clicked the box. Should I add another page under SQL Blacklist with description how it works now?

Regarding the tests - before my PR there were a bunch of scattered tests, validating Blacklist parsing functionality. Should I add some more validating Text -> statement parsing? Or should I move my tests under test/sql?

@SHaaD94 SHaaD94 requested a review from a team as a code owner January 13, 2025 13:20
@SHaaD94 SHaaD94 force-pushed the forked_origin_main branch from 17f39e3 to ce900f3 Compare January 13, 2025 13:22
@murphyatwork murphyatwork enabled auto-merge (squash) January 14, 2025 05:21
@murphyatwork
Copy link
Contributor

some advices:

  1. it will change the behavior, so need to check the box(we will mention it in the release note). as well it's better the change the document correspondingly.
  2. it's better to add some basic sql tests(test/sql/), which is easier to maintain and understand

Thanks, clicked the box. Should I add another page under SQL Blacklist with description how it works now?

Regarding the tests - before my PR there were a bunch of scattered tests, validating Blacklist parsing functionality. Should I add some more validating Text -> statement parsing? Or should I move my tests under test/sql?

Looks like we need another document page to describe the overall functionality, and the difference of behavior across versions. NVM, our document team can handle it since you've checked the box

@SHaaD94
review fixes
208fde5 
Signed-off-by: Evgeniy Zuykin <[email protected]>
auto-merge was automatically disabled January 14, 2025 08:44

Head branch was pushed to by a user without write access

@SHaaD94 SHaaD94 force-pushed the forked_origin_main branch from ce900f3 to 208fde5 Compare January 14, 2025 08:44
@SHaaD94
Copy link
Contributor Author

SHaaD94 commented Jan 14, 2025

Hi, @murphyatwork. I added some more tests to make coverage bot happy, would you mind enabling auto merge again?

Thanks.

@murphyatwork murphyatwork enabled auto-merge (squash) January 15, 2025 02:14
@SHaaD94 SHaaD94 force-pushed the forked_origin_main branch from 72acf1f to b443984 Compare January 15, 2025 05:25
@SHaaD94
Copy link
Contributor Author

SHaaD94 commented Jan 15, 2025

@SHaaD94 can you sign the CLA ? otherwise the CI would not run automatically

My bad, repushed with a correct author.

@murphyatwork murphyatwork enabled auto-merge (squash) January 15, 2025 10:41
@murphyatwork murphyatwork disabled auto-merge January 15, 2025 10:46
@murphyatwork murphyatwork enabled auto-merge (squash) January 15, 2025 10:46
gengjun-git
gengjun-git previously approved these changes Jan 15, 2025
View reviewed changes
@SHaaD94
Fix tests
0e0292b 
Signed-off-by: Evgeniy Zuykin <[email protected]>
@SHaaD94
Empty, to retriger CI
d482180 
Signed-off-by: Evgeniy Zuykin <[email protected]>
auto-merge was automatically disabled January 16, 2025 03:42

Head branch was pushed to by a user without write access

@SHaaD94 SHaaD94 dismissed stale reviews from gengjun-git and murphyatwork via d482180 January 16, 2025 03:42
@SHaaD94 SHaaD94 force-pushed the forked_origin_main branch from b443984 to d482180 Compare January 16, 2025 03:42
murphyatwork
murphyatwork previously approved these changes Jan 16, 2025
View reviewed changes
gengjun-git
gengjun-git previously approved these changes Jan 16, 2025
View reviewed changes
@SHaaD94
Attempt to fix failing test
0ebe3a8 
Signed-off-by: Evgeniy Zuykin <[email protected]>
@SHaaD94 SHaaD94 dismissed stale reviews from gengjun-git and murphyatwork via 0ebe3a8 January 16, 2025 08:48
@SHaaD94 SHaaD94 requested a review from murphyatwork January 16, 2025 09:01
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
15 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@github-actions GitHub Actions
Copy link

[Java-Extensions Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

@github-actions GitHub Actions
Copy link

[FE Incremental Coverage Report]

✅ pass : 83 / 86 (96.51%)

file detail

path covered_line new_line coverage not_covered_line_detail
🔵 com/starrocks/server/GlobalStateMgr.java 2 4 50.00% [1570, 1770]
🔵 com/starrocks/meta/SqlBlackList.java 48 49 97.96% [56]
🔵 com/starrocks/persist/DeleteSqlBlackLists.java 3 3 100.00% []
🔵 com/starrocks/sql/ast/DelSqlBlackListStmt.java 1 1 100.00% []
🔵 com/starrocks/persist/SqlBlackListPersistInfo.java 4 4 100.00% []
🔵 com/starrocks/qe/StmtExecutor.java 7 7 100.00% []
🔵 com/starrocks/persist/EditLog.java 11 11 100.00% []
🔵 com/starrocks/sql/ast/AddSqlBlackListStmt.java 1 1 100.00% []
🔵 com/starrocks/persist/EditLogDeserializer.java 2 2 100.00% []
🔵 com/starrocks/persist/metablock/SRMetaBlockID.java 1 1 100.00% []
🔵 com/starrocks/qe/ShowExecutor.java 3 3 100.00% []

@github-actions GitHub Actions
Copy link

[BE Incremental Coverage Report]

✅ pass : 0 / 0 (0%)

@murphyatwork murphyatwork enabled auto-merge (squash) January 16, 2025 11:30
@murphyatwork murphyatwork merged commit 4ff8cd1 into StarRocks:main Jan 16, 2025
45 checks passed
@wanpengfei-git wanpengfei-git mentioned this pull request Jan 16, 2025
Closed
1 task
@SHaaD94 SHaaD94 deleted the forked_origin_main branch January 16, 2025 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@starrocks-cr starrocks-cr[bot] starrocks-cr[bot] left review comments

@murphyatwork murphyatwork murphyatwork approved these changes

@gengjun-git gengjun-git gengjun-git approved these changes

@HangyuanLiu HangyuanLiu HangyuanLiu approved these changes

Assignees

@SHaaD94 SHaaD94

@HangyuanLiu HangyuanLiu

@gengjun-git gengjun-git

Labels
behavior_changed META-REVIEW version:3.5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Persistent SQL blacklist
6 participants
@SHaaD94 @CLAassistant @murphyatwork @HangyuanLiu @gengjun-git @wanpengfei-git