Add single node MetadataStore implementation #1291

tillrohrmann · 2024-03-19T22:30:06Z

The implementation is based on a single node which uses Restate's
networking and stores the kv pairs durably in a RocksDB instance.

This fixes #1284.

This PR is based on #1288.

AhmedSoliman

Very Impressive! I feel bad on how slow I'm moving on the local loglet in comparison 😆
I've left a few minor nits but in general it's good to go.

crates/types/src/errors.rs

crates/types/Cargo.toml

crates/metadata-store/Cargo.toml

AhmedSoliman · 2024-03-20T09:22:01Z

crates/metadata-store/src/lib.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`].
    async fn put(
        &self,
-        key: &str,
+        key: ByteString,


Curious to know the benefits of using ByteString here, I'm under the impression that pretty much all keys are static strings.

The reason why I switched to ByteString is that I needed an owned type for the restate_node_protocol::metadata_store::Put message and I wanted to avoid an extra clone operation for &'static str keys. With ByteString::from_static(&'static str) this should be possible. Moreover, I didn't want to change the traits type to &'static str which seemed too limiting for users. This is probably premature optimization at its best 😅

Thanks for explaining. I think at some point we should explore using smol_str or smartstring for most of our internal strings.

AhmedSoliman · 2024-03-20T09:32:13Z

crates/metadata-store/src/rocksdb/client.rs

+    async fn request(&self, kind: RequestKind) -> Result<ResponseKind, RequestError> {
+        let request_id = self.next_request_id();
+
+        // eventual clean up of closed response senders
+        if request_id >= self.state.last_clean_up.load(Ordering::Relaxed) + 1000 {
+            self.retain_open_response_senders()
+        }


It sounds like pretty soon we will need to write an abstraction for in-flight request/response tracking. This will be a common pattern as we build more networked components.

Yes indeed. This would be great to solve generically :-)

crates/metadata-store/src/rocksdb/server.rs

AhmedSoliman · 2024-03-20T09:59:48Z

crates/metadata-store/src/rocksdb/server.rs

+    fn delete(&self, key: &ByteString, precondition: &Precondition) -> Result<()> {
+        match precondition {
+            Precondition::None => self.delete_kv_pair(key),
+            // this condition does not really make sense for the delete operation
+            Precondition::DoesNotExist => {
+                let current_version = self.get_version(key)?;
+
+                if current_version.is_none() {
+                    // nothing to do
+                    Ok(())
+                } else {
+                    Err(Error::kv_pair_exists())
+                }
+            }
+            Precondition::MatchesVersion(version) => {
+                let current_version = self.get_version(key)?;
+
+                if current_version.as_ref() == Some(version) {
+                    self.delete_kv_pair(key)
+                } else {
+                    Err(Error::version_mismatch(version, current_version.as_ref()))
+                }
+            }
+        }
+    }


Disclaimer: It's unlikely that we will need delete() so everything I write here is not necessary.

A note on deleting versioned data. Removing versioned data require that we leave a tombstone with the latest version that was used. This ensures that if we ever re-put the data, we'd be sure that the new value picks up a higher version number.

This is true. Your comment actually touches upon another aspect that I would like to bring up to discuss: At the moment the metadata store implementation does not enforce that newly inserted values have a strictly larger version than a previously stored value. As long as the precondition is fulfilled, users can put a value with an arbitrary version. This puts a stronger burden on the user to set the versions correctly. If we enforce this invariant, then we would additionally need the admin command set to set an arbitrary versioned value which is currently possible via put.

I wouldn't worry so much about versions going backwards, in fact, it might come handy in some emergency tooling because if this happens, it's a bug.

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

tillrohrmann · 2024-03-20T21:48:17Z

@AhmedSoliman I've pushed a new commit (the last one) which changes the MetadataStore trait and implementation to require strictly monotonic versions when putting new items. It restricts a bit the flexibility of the metadatastore while it enforces that versions are always increasing. Not entirely sure whether it isn't actually a disimprovement.

AhmedSoliman · 2024-03-22T16:09:54Z

crates/grpc-util/Cargo.toml

@@ -0,0 +1,17 @@
+[package]
+name = "restate-grpc-util"


Could that be part of restate-network or would that be needed in crates that don't depend on restate-network already?

Currently, the metadata-store crate needs this functionality and does not depend on restate-network.

crates/grpc-util/src/lib.rs

AhmedSoliman · 2024-03-22T16:22:04Z

crates/metadata-store/src/lib.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`].
    async fn put(
        &self,
-        key: &str,
+        key: ByteString,


Thanks for explaining. I think at some point we should explore using smol_str or smartstring for most of our internal strings.

AhmedSoliman · 2024-03-22T16:24:31Z

crates/metadata-store/src/rocksdb/server.rs

+///
+/// In order to avoid issues arising from concurrency, we run the metadata
+/// store in a single thread.
+pub struct RocksDBMetadataStore<N> {


Good call. let's refine the naming as we progress forward. Eventually single/multinode will be merged I suppose.

crates/metadata-store/src/rocksdb/server.rs

AhmedSoliman · 2024-03-22T16:36:19Z

crates/metadata-store/src/rocksdb/server.rs

+    fn delete(&self, key: &ByteString, precondition: &Precondition) -> Result<()> {
+        match precondition {
+            Precondition::None => self.delete_kv_pair(key),
+            // this condition does not really make sense for the delete operation
+            Precondition::DoesNotExist => {
+                let current_version = self.get_version(key)?;
+
+                if current_version.is_none() {
+                    // nothing to do
+                    Ok(())
+                } else {
+                    Err(Error::kv_pair_exists())
+                }
+            }
+            Precondition::MatchesVersion(version) => {
+                let current_version = self.get_version(key)?;
+
+                if current_version.as_ref() == Some(version) {
+                    self.delete_kv_pair(key)
+                } else {
+                    Err(Error::version_mismatch(version, current_version.as_ref()))
+                }
+            }
+        }
+    }


I wouldn't worry so much about versions going backwards, in fact, it might come handy in some emergency tooling because if this happens, it's a bug.

crates/metadata-store/proto/metadata_store_svc.proto

BindAddress is the server side equivalent of the AdvertisedAddress and allows to specify where to bind the server to.

The restate-grpc-util crate contains helper functions for inter- acting with grpc services.

The generic hyper server run util takes a hyper service and a bind address. It will let the server bind to the specified bind address.

This commit removes the increment_version method from the Versioned trait since it is only used for testing purposes.

tillrohrmann · 2024-03-22T17:37:16Z

Thanks for your review @AhmedSoliman. I've addressed your comments and pushed the final version. Merging this PR once GHA gives green light.

The implementation is based on a single node which uses gRPC for communication and stores the kv pairs durably in a RocksDB instance. This fixes restatedev#1284.

AhmedSoliman approved these changes Mar 20, 2024

View reviewed changes

tillrohrmann force-pushed the issues/1284 branch from cb83864 to e08286f Compare March 20, 2024 21:46

tillrohrmann force-pushed the issues/1284 branch 2 times, most recently from 8960c9c to 2dcc529 Compare March 22, 2024 13:08

tillrohrmann requested a review from AhmedSoliman March 22, 2024 13:09

tillrohrmann force-pushed the issues/1284 branch from 2dcc529 to 3d1d3c3 Compare March 22, 2024 13:12

AhmedSoliman approved these changes Mar 22, 2024

View reviewed changes

tillrohrmann added 6 commits March 22, 2024 18:35

Introduce BindAddress

a893e72

BindAddress is the server side equivalent of the AdvertisedAddress and allows to specify where to bind the server to.

Introduce restate-grpc-util crate

18dabcd

The restate-grpc-util crate contains helper functions for inter- acting with grpc services.

Add generic hyper server run util to grpc-util

987ceef

The generic hyper server run util takes a hyper service and a bind address. It will let the server bind to the specified bind address.

Add GenericError to restate-types

5fd7c38

Rename restate-type feature mocks into test-util

3a77d13

Remove increment_version from Versioned trait

7507516

This commit removes the increment_version method from the Versioned trait since it is only used for testing purposes.

tillrohrmann force-pushed the issues/1284 branch from 3d1d3c3 to 8d99db9 Compare March 22, 2024 17:36

Add single node MetadataStore implementation

ba7893c

The implementation is based on a single node which uses gRPC for communication and stores the kv pairs durably in a RocksDB instance. This fixes restatedev#1284.

tillrohrmann force-pushed the issues/1284 branch from 8d99db9 to ba7893c Compare March 22, 2024 17:39

tillrohrmann merged commit ba7893c into restatedev:main Mar 23, 2024
5 checks passed

tillrohrmann deleted the issues/1284 branch March 23, 2024 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add single node MetadataStore implementation #1291

Add single node MetadataStore implementation #1291

tillrohrmann commented Mar 19, 2024

AhmedSoliman left a comment

AhmedSoliman Mar 20, 2024

tillrohrmann Mar 20, 2024

AhmedSoliman Mar 22, 2024

AhmedSoliman Mar 20, 2024

tillrohrmann Mar 20, 2024

AhmedSoliman Mar 20, 2024

tillrohrmann Mar 20, 2024

AhmedSoliman Mar 22, 2024

tillrohrmann commented Mar 20, 2024 •

edited

Loading

AhmedSoliman Mar 22, 2024

tillrohrmann Mar 22, 2024

AhmedSoliman Mar 22, 2024

AhmedSoliman Mar 22, 2024

AhmedSoliman Mar 22, 2024

tillrohrmann commented Mar 22, 2024

Add single node MetadataStore implementation #1291

Add single node MetadataStore implementation #1291

Conversation

tillrohrmann commented Mar 19, 2024

AhmedSoliman 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

Choose a reason for hiding this comment

tillrohrmann commented Mar 20, 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

tillrohrmann commented Mar 22, 2024

tillrohrmann commented Mar 20, 2024 •

edited

Loading