-
Notifications
You must be signed in to change notification settings - Fork 44
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
Auto provision #2058
Auto provision #2058
Conversation
c9173a5
to
03ef822
Compare
03ef822
to
2ec71ed
Compare
self.task_center.clone(), | ||
self.metadata_store_client.clone(), | ||
self.networking.clone(), | ||
) | ||
.await?; | ||
|
||
let mut logs_controller = LogsController::init( | ||
configuration, |
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.
Not a blocker, but I'd have loved if we rely more on Live configuration for a more reactive experience.
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.
Once the LogsController
depends on values in the Configuration
apart from the init
phase, then this sounds like a good idea.
async fn init_partition_table(&mut self) -> anyhow::Result<()> { | ||
let configuration = self.configuration.live_load(); | ||
|
||
let partition_table = retry_on_network_error( |
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.
nit: Might be worth logging that we are initializing
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.
Yes, will add it.
config.common.cluster_name(), | ||
)))?; | ||
} | ||
// todo write bootstrap state |
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.
what should we expect to do here?
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.
This is not fully clear to me yet. It could be the initial default log provider or whether to auto provision partitions if this needs to be agreed upon.
/// # Automatically provision number of configured partitions | ||
/// | ||
/// If this option is set to `false`, then one needs to manually write a partition table to | ||
/// the metadata store. Without a partition table, the cluster will not start. | ||
#[clap(long, global = true)] | ||
pub auto_provision_partitions: Option<bool>, |
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 we will need to review the ergonomics of this process but I wouldn't block merging this.
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.
Sounds good :-)
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.
Very nice. I'm fine with this as a temporary design until we discuss a more ergonomic way for single-node vs. cluster setup.
Thanks for the review @AhmedSoliman. I will address your comments and then merge this PR once GHA gives green light. |
2ec71ed
to
b4182b4
Compare
This also adds the auto-provision option which tells the cluster controller whether to create an intially empty partition table or a partition table with as many partitions as the configured bootstrap partitions. This fixes restatedev#2084.
b4182b4
to
c4e958a
Compare
Adds a feature to disable the auto-provisioning of partitions by the cluster controller. To disable the auto-provisioning, you can set the
auto-provision-partitions = false
in the configuration or on the CLI via--auto-provision-partitions=false
.This PR is based on #2053