Conversation
Automatically migrates logs metadata to replicated loglet in single-node configuration. This will cause Bifrost to extend the chain with new replicated loglet segments. The commit also removes an old logs initialization path which has sinced been replaced by the provisioning task.
|
|
||
| /// Creates empty metadata if none exists for bifrost and publishes it to metadata | ||
| /// manager. | ||
| pub async fn init_metadata(&self) -> Result<(), Error> { |
There was a problem hiding this comment.
I believe this path can't be reached anymore now that we wait for an initial logs version on startup.
| if !config.has_role(Role::LogServer) { | ||
| // This means that roles were explicitly set; we can not migrate to replicated. | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Does this warrant a log line to nudge the user towards activating it? I suspect it wouldn't get noticed unless very prominent.
There was a problem hiding this comment.
I don't think so. What you have now is good.
|
Unfortunately, as it stands, we can't perform auto migration to replicated loglet without ensuring that the log has been trimmed after creating the new segments. This is to ensure that users can expand this standalone setup to a cluster safely. |
AhmedSoliman
left a comment
AhmedSoliman
left a comment
There was a problem hiding this comment.
Very nice work. Thank you for doing a neat job switching our defaults and handling automatic migration as well.
🚢
| if !config.has_role(Role::LogServer) { | ||
| // This means that roles were explicitly set; we can not migrate to replicated. | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
I don't think so. What you have now is good.
|
|
||
| /// Creates empty metadata if none exists for bifrost and publishes it to metadata | ||
| /// manager. | ||
| pub async fn init_metadata(&self) -> Result<(), Error> { |
| // Why not ProviderConfiguration::from_configuration(config)? Using the default | ||
| // replicated loglet provider configuration is a safer alternative as there might be | ||
| // untested config changes that would fail later, while this is an appropriate | ||
| // successor to the pre-1.4.0 default local loglet config for single nodes. Anything | ||
| // more complex should be done as an explicit reconfiguration by the operator. | ||
| let logs_configuration = LogsConfiguration { | ||
| default_provider: ProviderConfiguration::default(), | ||
| }; |
| match result { | ||
| Ok(_) => {} | ||
| Err(err) => { | ||
| warn!( | ||
| "Failed to update the default loglet configuration in metadata: {}", | ||
| err | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: if let Err(err) = result { ... } is more appropriate if you don't return anything from the Ok() case.
| /// | ||
| /// Default: Local | ||
| /// Default: Replicated | ||
| pub default_provider: ProviderKind, |
There was a problem hiding this comment.
bonus points if we can follow up with a PR that skips serialization if it's the default value already. Same for metadata.
Addressed through documentation for now #3410 while I think about how best to incorporate this into auto-trimming. |
2 participants
Automatically migrates logs metadata to replicated loglet in single-node configuration. This will cause Bifrost to extend the chain with new replicated loglet segments. The commit also removes an old logs initialization path which has sinced been replaced by the provisioning task.
Closes #3129
Testing notes
1.4.0-dev: fresh start
✅ Starts with replicated loglet as expected:
1.3.2 -> 1.4.0-dev: default loglet config automatic migration
% npx @restatedev/[email protected] --node-name n1Stop and re-start with 1.4.0-dev:
% restate-server --node-name n1✅ Migrated as expected:
1.3.2 -> 1.4.0-dev: no migration if loglet type is explicitly configured
RESTATE_BIFROST__DEFAULT_PROVIDER="local" restate-server --node-name n1✅ No migration as expected:
1.3.2 -> 1.4.0-dev: no migration if
log-serverrole is not enabledrestate-server --node-name n1 --roles=worker,admin,metadata-server✅ No migration as expected: