-
Notifications
You must be signed in to change notification settings - Fork 799
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
feat: impl WarehouseOptions
for Private Task
#18404
Conversation
b5b36df
to
e33c7df
Compare
There was a problem hiding this 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.
e33c7df
to
959d837
Compare
There was a problem hiding this 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.
There was a problem hiding this 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:complete! all files reviewed, all discussions resolved (waiting on @sundy-li and @zhang2014)
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)
Tests
Type of change
This change is