Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add EpochTable and integrate with existing code #1332

Merged
merged 7 commits into from
Apr 2, 2024
Merged

Add EpochTable and integrate with existing code #1332

merged 7 commits into from
Apr 2, 2024

Conversation

tillrohrmann
Copy link
Contributor

This PR introduces the EpochTable which is used to record the latest leader epoch for a given PartitionId and the node assignment. The EpochTable is updated by the cluster controller upon attachment of a new node. The last attaching node will get all leaderships assigned. Moreover, this PR updates the Worker to react to EpochTable and PartitionTable changes which require to run a different set of partition processors or make a different set of partition processors leaders. Last but not least, this PR updates the metadata update logic which now also supports sending epoch and partition table updates.

This fixes #1323

@tillrohrmann
Copy link
Contributor Author

One thing I noticed while working on integrating the metadata structures is that one needs to pay a bit of attention to the fact that one might have received an update for the EpochTable but not necessarily for the PartitionTable since both structures are versioned independently.

@AhmedSoliman
Copy link
Contributor

One thing I noticed while working on integrating the metadata structures is that one needs to pay a bit of attention to the fact that one might have received an update for the EpochTable but not necessarily for the PartitionTable since both structures are versioned independently.

What's the risk in this? In my mind, the PartitionTable needs to be on a single shared register, but epoch metadata don't actually even need to be (we can have a single metadata key per partition-id). Additionally, EpochMetadata don't need to be automatically synchronized, unless I'm missing something.

@tillrohrmann
Copy link
Contributor Author

What's the risk in this? In my mind, the PartitionTable needs to be on a single shared register, but epoch metadata don't actually even need to be (we can have a single metadata key per partition-id). Additionally, EpochMetadata don't need to be automatically synchronized, unless I'm missing something.

No concrete risk atm. Just something to be aware of (more of a note to myself). I did include the EpochMetadata in the metadata propagation because I thought that knowing who is the current leader for a given partition_id could be useful.

AhmedSoliman
AhmedSoliman reviewed Mar 28, 2024
View reviewed changes
crates/core/src/metadata/mod.rs Outdated Show resolved Hide resolved
crates/node-protocol/src/metadata.rs Outdated Show resolved Hide resolved
crates/core/src/metadata/mod.rs Outdated Show resolved Hide resolved
crates/node-services/proto/cluster_ctrl_svc.proto Outdated Show resolved Hide resolved
crates/node/src/lib.rs Outdated Show resolved Hide resolved
crates/types/src/epoch_table.rs Outdated Show resolved Hide resolved
crates/types/src/epoch_table.rs Outdated Show resolved Hide resolved
crates/worker/src/lib.rs
let shutdown = cancellation_watcher();
tokio::pin!(shutdown);

let mut partition_processor_manager = PartitionProcessorManager::new(
Copy link
Contributor

Choose a reason for hiding this comment

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

🥳

crates/worker/src/lib.rs Outdated Show resolved Hide resolved
crates/worker/src/lib.rs Outdated Show resolved Hide resolved
@tillrohrmann
Copy link
Contributor Author

Thanks for the review @AhmedSoliman. I've pushed an update to address your comments.

@tillrohrmann tillrohrmann mentioned this pull request Apr 2, 2024
Merged
AhmedSoliman
AhmedSoliman approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@AhmedSoliman AhmedSoliman left a comment

Choose a reason for hiding this comment

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

Great stuff!

crates/types/src/epoch.rs Outdated Show resolved Hide resolved
crates/types/src/epoch.rs Outdated Show resolved Hide resolved
crates/types/src/epoch.rs
}

pub fn epoch(&self) -> LeaderEpoch {
// todo think about aligning Version and LeaderEpoch types
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe make use of types/src/logs.rs (trait SequenceNumber) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, this could also be a solution. The conversion Version -> u32 -> u64 -> LeaderEpoch is not super nice but also not incorrect at the moment.

crates/node/src/lib.rs Outdated Show resolved Hide resolved
crates/node/src/lib.rs Outdated Show resolved Hide resolved
crates/node/src/network_server/handler/cluster_ctrl.rs Outdated Show resolved Hide resolved
@tillrohrmann
Copy link
Contributor Author

Thanks a lot for the review @AhmedSoliman. I've addressed your comments. Merging this PR once GHA gives green light.

@tillrohrmann tillrohrmann force-pushed the issues/1323 branch 2 times, most recently from 9f86e92 to b18b29e Compare April 2, 2024 15:20
@tillrohrmann
Add basic Epoch data structure
77937bc
@tillrohrmann
Integrate Epoch with worker
293b81d 
This fixes restatedev#1323.
@tillrohrmann
Obtain number of partitions from partition table
0731bb5 
Let Bifrost obtain the number of partitions from the available
partition table.
@tillrohrmann
Introduce PartitionProcessorManager
f8f1358 
The PartitionProcessorManager is responsible for managing the lifecycle
of PartitionProcessors. The set of partition processors can be modified
by providing a PartitionProcessorPlan that contains actions how to change
the running set of PartitionProcessors. At the moment only starting
partition processors is supported.
@tillrohrmann
Support sending partition table metadata to peers
8b3fce9
@tillrohrmann
Let MetadataContainer derive derive_more::From
6207556
@tillrohrmann
Make PartitionTable metadata optional in metadata
f133a78 
This commit changes the APIs for obtaining the PartitionTable metadata
to optional to underline the contract that a PartitionTable might not
be loaded.
@tillrohrmann tillrohrmann merged commit f133a78 into restatedev:main Apr 2, 2024
4 of 5 checks passed
@tillrohrmann tillrohrmann deleted the issues/1323 branch April 2, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AhmedSoliman AhmedSoliman AhmedSoliman approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Epoch metadata and store it in metadata store
2 participants
@tillrohrmann @AhmedSoliman