Skip to content

feat: impl WarehouseOptions for Private Task #18404

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

Merged
merged 4 commits into from
Jul 29, 2025

Conversation

KKould
Copy link
Member

@KKould KKould commented Jul 22, 2025

I hereby agree to the terms of the CLA available at: https://docs.databend.com/dev/policies/cla/

Summary

Private Task supports WarehouseOptions for resource control(via cluster_id)

CREATE TASK my_task_1 WAREHOUSE = 'wh1' SCHEDULE = 3 SECOND AS insert into t1 values(1)

Tests

  • Unit Test
  • Logic Test
  • Benchmark Test
  • No Test - test-private-task-warehouse.sh (manually test the script)

Type of change

  • Bug Fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Breaking Change (fix or feature that could cause existing functionality not to work as expected)
  • Documentation Update
  • Refactoring
  • Performance Improvement
  • Other (please describe):

This change is Reviewable

@KKould KKould self-assigned this Jul 22, 2025
@github-actions github-actions bot added the pr-feature this PR introduces a new feature to the codebase label Jul 22, 2025
@KKould KKould force-pushed the feat/private_task_warehouse branch 5 times, most recently from b5b36df to e33c7df Compare July 25, 2025 08:35
@KKould KKould requested review from zhang2014 and sundy-li July 25, 2025 09:01
@KKould KKould marked this pull request as ready for review July 25, 2025 09:35
@KKould KKould requested a review from drmingdrmer as a code owner July 25, 2025 09:35
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sundy-li and @zhang2014)


src/meta/proto-conv/tests/it/v139_task_message.rs line 26 at r1 (raw file):

#[test]
fn test_decode_v139_task_message() -> anyhow::Result<()> {

I found that FromToProto for TaskMessage is not tested in the version it is introduced.

Are you sure this TaskMessage can load the previous version TaskMessage::Delete(String) with TaskMessage::Delete(DeleteTask)?

Maybe you should fire another PR to add the backward compatibility test for the previous version of TaskMessage::Delete before merge this PR.

@KKould KKould force-pushed the feat/private_task_warehouse branch from e33c7df to 959d837 Compare July 29, 2025 08:08
@KKould KKould requested a review from drmingdrmer July 29, 2025 08:08
Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 6 files at r2, all commit messages.
Reviewable status: 9 of 12 files reviewed, 2 unresolved discussions (waiting on @KKould, @sundy-li, and @zhang2014)


src/meta/proto-conv/src/task_from_to_protobuf_impl.rs line 159 at r2 (raw file):

                    mt::TaskMessage::ScheduleTask(mt::Task::from_pb(task)?)
                }
                Message::DeleteTask(task_name) => mt::TaskMessage::DeleteTask(task_name, None),

This should be without None and keep the same as previous structure to provide compatibility.

Copy link
Member

@drmingdrmer drmingdrmer left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 6 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @sundy-li and @zhang2014)

@sundy-li sundy-li merged commit 73bfbb9 into databendlabs:main Jul 29, 2025
87 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants