Externa metadata store #1838

muhamadazmy · 2024-08-14T13:39:56Z

This PR adds support to external metadata store. Right now we only implement the etcd store.

Support configuration for other metadata store types
Implement the etcd store
Testing
- ~~Unit tests~~

Note:
The Testing above is manual testing that involves an etcd cluster. The etcd nodes are randomly stopped or killed and looks like restate operations resumes normally with no issue. Unless the number of down etcd node is bigger than the quorum then of course it fails to write changes. This is restored once enough nodes are back

Fixes #1829

The implementation uses an external etcd client to implement the basic Metadata store functionality

tillrohrmann

Thanks a lot for creating this PR @muhamadazmy. Having an etcd integration is really cool :-) The changes look good to me.

I think that I have given you bad advice on the delete call specification. I think it is fine if we delete the entry and don't leave a tombstone behind. If users need it, then they can do it themselves.

Once you have updated this bit, then let me know and I'll give the PR another pass.

tillrohrmann · 2024-08-15T12:25:00Z

Cargo.toml

@@ -194,6 +194,7 @@ tracing-test = { version = "0.2.5" }
 ulid = { version = "1.1.0" }
 url = { version = "2.5" }
 uuid = { version = "1.3.0", features = ["v7", "serde"] }
+etcd-client = { version = "0.14" }


We try to order dependencies alphabetically. Unfortunately, there is no good tooling for it.

tillrohrmann · 2024-08-15T12:57:54Z

crates/core/Cargo.toml

 tower = { workspace = true }
 tracing = { workspace = true }
+etcd-client = { workspace = true }


Same here with the alphabetical sorting.

tillrohrmann · 2024-08-15T13:01:50Z

crates/core/src/metadata_store/providers/etcd.rs

+
+impl From<etcd_client::Error> for ReadError {
+    fn from(value: etcd_client::Error) -> Self {
+        Self::Network(value.into())


Are all etcd_client::Error network related or could there be an invalid argument or something different? Maybe we also need to extend ReadError if it is not expressive enough to return different kind of errors.

Yeah, i had to made a choice either to assume it's all network, or extend ReadError.

Maybe we can a variant StoreError(GenericError) for any possible store error that can't be categorized to a specific Read/WriteError ?

tillrohrmann · 2024-08-15T13:05:40Z

crates/core/src/metadata_store/providers/etcd.rs

+//
+// The correct solution is of course to make Version u128.
+// What we do here is ONLY relying on the "version" of the key combined with the fact that we never actually delete the key
+// but instead put a "tombstone" in place so version doesn't reset


Suggested change

// but instead put a "tombstone" in place so version doesn't reset

// but instead put a "tombstone" in place so version doesn't reset.

Maybe also add that we are only using the lower 32 bits of the Etcd version and fail once the version is larger than u32::MAX.

tillrohrmann · 2024-08-15T13:07:13Z

crates/core/src/metadata_store/providers/etcd.rs

+impl ToVersion for &KeyValue {
+    fn to_version(self) -> Result<Version, ReadError> {
+        if self.version() > u32::MAX as i64 {
+            return Err(ReadError::Codec(


Maybe this is more an internal error than a codec error?

tillrohrmann · 2024-08-15T13:54:11Z

crates/core/src/metadata_store/providers/etcd.rs

+    /// Gets the value and its current version for the given key. If key-value pair is not present,
+    /// then return [`None`].


At least with Rust rover, you don't need to copy the RustDoc on the implementation, since the IDE will still display it. Then we only have one place where we need to update the RustDocs.

tillrohrmann · 2024-08-15T14:46:31Z

crates/core/src/metadata_store/providers/etcd.rs

+        let mut client = self.client.kv_client();
+        let mut response = client.get(key.into_bytes(), None).await?;
+
+        // return first value because this suppose to be an exact match


Suggested change

// return first value because this suppose to be an exact match

// return first value because this is supposed to be an exact match

tillrohrmann · 2024-08-15T14:48:13Z

crates/core/src/metadata_store/providers/etcd.rs

+    /// Puts the versioned value under the given key following the provided precondition. If the
+    /// precondition is not met, then the operation returns a [`WriteError::PreconditionViolation`].
+    ///
+    /// NOTE: this implementation disregard that version attached on the value and depends only on the


Suggested change

/// NOTE: this implementation disregard that version attached on the value and depends only on the

/// NOTE: This implementation disregards the version attached to the value and depends only on the

tillrohrmann · 2024-08-15T14:50:25Z

crates/core/src/metadata_store/providers/etcd.rs

+                    .put_with_version(
+                        &mut client,
+                        key.into_bytes(),
+                        Vec::default().into(),


Sorry for giving you bad advice. I think it is ok to implement the delete operation by deleting the complete entry. If users of the MetadataStore want to keep the version around, then they need to leave a tombstone behind.

tillrohrmann · 2024-08-15T14:53:32Z

crates/types/src/config/common.rs

+    /// Connects to another node that is running Metadata Store
+    /// over gRPC.
+    /// The remote node must run with MetadataStore Role


Maybe

Suggested change

/// Connects to another node that is running Metadata Store

/// over gRPC.

/// The remote node must run with MetadataStore Role

/// Connects to an embedded metadata store that is run by nodes that run with the MetadataStore role.

muhamadazmy · 2024-08-16T08:44:02Z

@tillrohrmann thank you so much for your review. I applied All the comments. I also changed how we delete keys from the etcd store which reduced the code drastically and hence some comments became irrelevant.

tillrohrmann

Great, nice work @muhamadazmy. The changes look good to me. I left a few minor comments. +1 for merging after resolving them.

tillrohrmann · 2024-08-16T09:31:46Z

crates/core/src/metadata_store/providers/etcd.rs

+
+    /// deletes a key only if and only if the current value in store
+    /// has the exact given version
+    async fn delete_if_has_version(


Suggested change

async fn delete_if_has_version(

async fn delete_if_version_matches(

tillrohrmann · 2024-08-16T09:32:09Z

crates/core/src/metadata_store/providers/etcd.rs

+
+    /// puts a key/value if and only if the current value in store
+    /// has the exact given version
+    async fn put_if_has_version(


Suggested change

async fn put_if_has_version(

async fn put_if_version_matches(

tillrohrmann · 2024-08-16T09:33:10Z

crates/core/src/metadata_store/providers/etcd.rs

+            // we keep an empty value in place of
+            // deleted keys to work around the version limitation


I guess the comment is now outdated.

tillrohrmann · 2024-08-16T09:33:46Z

crates/core/src/metadata_store/providers/etcd.rs

+            // we keep an empty value in place of deleted
+            // keys to work around the version limitation


Yes, definitely! The entire implementation can be improved now to not relay on the value being empty. Thanks for pointing this out

tillrohrmann · 2024-08-16T09:36:36Z

crates/metadata-store/src/local/tests.rs

+    let metadata_store_address = if let config::MetadataStore::Embedded { address } =
+        metadata_store_client_options.metadata_store_client
+    {
+        address.clone()
+    } else {
+        unimplemented!()
+    };


For cases where you expect a single variant, you can use let_assert!(config::MetadataStore::Embedded { address } = metadata_store_client_options.metadata_store_client) to destructure the type.

tillrohrmann · 2024-08-16T09:38:05Z

crates/types/src/config/common.rs

-    #[cfg_attr(feature = "schemars", schemars(with = "String"))]
-    pub metadata_store_address: AdvertisedAddress,
+    /// Metadata store server to bootstrap the node from.
+    pub metadata_store_client: MetadataStore,


Maybe rename MetadataStore into MetadataStoreClient.

muhamadazmy and others added 12 commits August 14, 2024 15:14

WIP: update meta-store config

361368f

update docstring

0894596

using socket address

c820975

update docstr

bc8956d

minor renames (#1)

4693432

WIP: implement a simple etcd store

f0af2f9

The implementation uses an external etcd client to implement the basic Metadata store functionality

we can cheaply get a kv client

ab16977

remove unused deps

7c08c7b

implementation of version from etc kv

526318f

implementaiton of version

9f26f20

first code complete implementation of etcd metastore

b677030

actually using the etcd client

f78aee7

muhamadazmy requested review from tillrohrmann and AhmedSoliman August 15, 2024 10:54

muhamadazmy marked this pull request as ready for review August 15, 2024 12:35

tillrohrmann reviewed Aug 15, 2024

View reviewed changes

muhamadazmy added 3 commits August 16, 2024 10:18

Applying PR comments

930eead

move comments

94cddeb

fix comment

8e939fc

tillrohrmann approved these changes Aug 16, 2024

View reviewed changes

Apply final PR comments

4c4ad52

muhamadazmy merged commit c82acb3 into restatedev:main Aug 16, 2024
10 checks passed

muhamadazmy deleted the externa-metadata-store branch August 16, 2024 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Externa metadata store #1838

Externa metadata store #1838

muhamadazmy commented Aug 14, 2024 •

edited

Loading

tillrohrmann left a comment •

edited

Loading

tillrohrmann Aug 15, 2024

tillrohrmann Aug 15, 2024

tillrohrmann Aug 15, 2024

muhamadazmy Aug 16, 2024 •

edited

Loading

tillrohrmann Aug 15, 2024

tillrohrmann Aug 15, 2024

tillrohrmann Aug 15, 2024

tillrohrmann Aug 15, 2024

tillrohrmann Aug 15, 2024

tillrohrmann Aug 15, 2024

tillrohrmann Aug 15, 2024

muhamadazmy commented Aug 16, 2024

tillrohrmann left a comment

tillrohrmann Aug 16, 2024

tillrohrmann Aug 16, 2024

tillrohrmann Aug 16, 2024

tillrohrmann Aug 16, 2024

muhamadazmy Aug 16, 2024

tillrohrmann Aug 16, 2024

tillrohrmann Aug 16, 2024

	// but instead put a "tombstone" in place so version doesn't reset
	// but instead put a "tombstone" in place so version doesn't reset.

		/// Gets the value and its current version for the given key. If key-value pair is not present,
		/// then return [`None`].

	// return first value because this suppose to be an exact match
	// return first value because this is supposed to be an exact match

	/// NOTE: this implementation disregard that version attached on the value and depends only on the
	/// NOTE: This implementation disregards the version attached to the value and depends only on the

	async fn delete_if_has_version(
	async fn delete_if_version_matches(

	async fn put_if_has_version(
	async fn put_if_version_matches(

		// we keep an empty value in place of
		// deleted keys to work around the version limitation

		// we keep an empty value in place of deleted
		// keys to work around the version limitation

Externa metadata store #1838

Externa metadata store #1838

Conversation

muhamadazmy commented Aug 14, 2024 • edited Loading

tillrohrmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhamadazmy Aug 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhamadazmy commented Aug 16, 2024

tillrohrmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhamadazmy commented Aug 14, 2024 •

edited

Loading

tillrohrmann left a comment •

edited

Loading

muhamadazmy Aug 16, 2024 •

edited

Loading