-
Notifications
You must be signed in to change notification settings - Fork 209
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
Cluster state refactor part 1 #560
Changes from all commits
b606c77
a157581
24d4830
c615fce
5ad27c0
490bda5
a802315
ff96bcd
694f6e2
91119e4
18790f4
41f228c
70e1bcf
96a8c9d
081b224
7ae9aaa
d34ecb5
ee2c9d0
3948c81
7062743
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,13 @@ type = "ballista_scheduler::state::backend::StateBackend" | |
doc = "The configuration backend for the scheduler, possible values: etcd, memory, sled. Default: sled" | ||
default = "ballista_scheduler::state::backend::StateBackend::Sled" | ||
|
||
[[param]] | ||
abbr = "c" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need both There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right now, no. But once part 2 is done, the goal is to be able to have different backends for cluster state and config (eg job) state. But maybe we expose that in the second PR so it is not so confusing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yahoNanJing are you okay to merge this as is? |
||
name = "cluster_backend" | ||
type = "ballista_scheduler::state::backend::StateBackend" | ||
doc = "The configuration backend for the scheduler cluster state, possible values: etcd, memory, sled. Default: sled" | ||
default = "ballista_scheduler::state::backend::StateBackend::Sled" | ||
|
||
[[param]] | ||
abbr = "n" | ||
name = "namespace" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -35,6 +35,7 @@ use log::{error, warn}; | |
|
||
use crate::scheduler_server::event::QueryStageSchedulerEvent; | ||
use crate::scheduler_server::query_stage_scheduler::QueryStageScheduler; | ||
use crate::state::backend::cluster::ClusterState; | ||
use crate::state::backend::StateBackendClient; | ||
use crate::state::executor_manager::{ | ||
ExecutorManager, ExecutorReservation, DEFAULT_EXECUTOR_TIMEOUT_SECONDS, | ||
|
@@ -69,12 +70,14 @@ impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan> SchedulerServer<T | |
pub fn new( | ||
scheduler_name: String, | ||
config_backend: Arc<dyn StateBackendClient>, | ||
cluster_state: Arc<dyn ClusterState>, | ||
codec: BallistaCodec<T, U>, | ||
config: SchedulerConfig, | ||
metrics_collector: Arc<dyn SchedulerMetricsCollector>, | ||
) -> Self { | ||
let state = Arc::new(SchedulerState::new( | ||
config_backend, | ||
cluster_state, | ||
default_session_builder, | ||
codec, | ||
scheduler_name.clone(), | ||
|
@@ -97,17 +100,53 @@ impl<T: 'static + AsLogicalPlan, U: 'static + AsExecutionPlan> SchedulerServer<T | |
} | ||
} | ||
|
||
pub fn with_session_builder( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not part of this PR, but I think it's wildly important we switch to struct update syntax or a builder pattern to avoid the combinatorial explosion of I did an issue search, I think it's covered by #479 ? |
||
scheduler_name: String, | ||
config_backend: Arc<dyn StateBackendClient>, | ||
cluster_backend: Arc<dyn ClusterState>, | ||
codec: BallistaCodec<T, U>, | ||
config: SchedulerConfig, | ||
session_builder: SessionBuilder, | ||
metrics_collector: Arc<dyn SchedulerMetricsCollector>, | ||
) -> Self { | ||
let state = Arc::new(SchedulerState::new( | ||
config_backend, | ||
cluster_backend, | ||
session_builder, | ||
codec, | ||
scheduler_name.clone(), | ||
config.clone(), | ||
)); | ||
let query_stage_scheduler = | ||
Arc::new(QueryStageScheduler::new(state.clone(), metrics_collector)); | ||
let query_stage_event_loop = EventLoop::new( | ||
"query_stage".to_owned(), | ||
config.event_loop_buffer_size as usize, | ||
query_stage_scheduler.clone(), | ||
); | ||
|
||
Self { | ||
scheduler_name, | ||
start_time: timestamp_millis() as u128, | ||
state, | ||
query_stage_event_loop, | ||
query_stage_scheduler, | ||
} | ||
} | ||
|
||
#[allow(dead_code)] | ||
pub(crate) fn with_task_launcher( | ||
scheduler_name: String, | ||
config_backend: Arc<dyn StateBackendClient>, | ||
cluster_backend: Arc<dyn ClusterState>, | ||
codec: BallistaCodec<T, U>, | ||
config: SchedulerConfig, | ||
metrics_collector: Arc<dyn SchedulerMetricsCollector>, | ||
task_launcher: Arc<dyn TaskLauncher>, | ||
) -> Self { | ||
let state = Arc::new(SchedulerState::with_task_launcher( | ||
config_backend, | ||
cluster_backend, | ||
default_session_builder, | ||
codec, | ||
scheduler_name.clone(), | ||
|
@@ -330,6 +369,7 @@ mod test { | |
use ballista_core::serde::BallistaCodec; | ||
|
||
use crate::scheduler_server::{timestamp_millis, SchedulerServer}; | ||
use crate::state::backend::cluster::DefaultClusterState; | ||
use crate::state::backend::sled::SledClient; | ||
|
||
use crate::test_utils::{ | ||
|
@@ -599,10 +639,12 @@ mod test { | |
scheduling_policy: TaskSchedulingPolicy, | ||
) -> Result<SchedulerServer<LogicalPlanNode, PhysicalPlanNode>> { | ||
let state_storage = Arc::new(SledClient::try_new_temporary()?); | ||
let cluster_state = Arc::new(DefaultClusterState::new(state_storage.clone())); | ||
let mut scheduler: SchedulerServer<LogicalPlanNode, PhysicalPlanNode> = | ||
SchedulerServer::new( | ||
"localhost:50050".to_owned(), | ||
state_storage.clone(), | ||
state_storage, | ||
cluster_state, | ||
BallistaCodec::default(), | ||
SchedulerConfig::default().with_scheduler_policy(scheduling_policy), | ||
Arc::new(TestMetricsCollector::default()), | ||
|
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.
Is it necessary to remove the blank after the ///
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.
No, not sure why it was removed in the generated code.
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.
Can you remind me again why the generated code is checked in? Where did we land on that?
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.
I think it was so the project played nicely with IDEs (some of which do not evaluate proc macros)