[wip] object store metastore #2309

igalshilman · 2024-11-18T10:44:00Z

No description provided.

tillrohrmann

Thanks for creating our object store based metadata store @igalshilman :-) It looks really nice! The implementation looks correct to me :-)

I left a few minor comments and questions. The one thing that is not fully clear to me is whether we want to clean up older versions or not. If not, would this be problematic for the get_latest_version which needs to list more and more versions?

tillrohrmann · 2024-11-22T11:05:39Z

crates/core/src/metadata_store/providers/objstore/glue.rs

+
+        let mut shutdown = pin::pin!(cancellation_watcher());
+
+        let delegate = match builder.build().await {


Can this await take a long time? If yes, then maybe select over the shutdown to also support shutdowns during this time.

Can this await take a long time?

not any more.

tillrohrmann · 2024-11-22T13:21:29Z

crates/core/src/metadata_store/providers/objstore/glue.rs

+            }
+        };
+
+        let mut refresh = tokio::time::interval(Duration::from_secs(2));


Probably something we want to make configurable at some point.

outdated, it doesn't exists any more.

tillrohrmann · 2024-11-22T13:23:10Z

crates/core/src/metadata_store/providers/objstore/glue.rs

+
+#[derive(Debug, Clone)]
+pub struct Client {
+    sender: tokio::sync::mpsc::UnboundedSender<Commands>,


Would a bounded sender also work?

tillrohrmann · 2024-11-22T13:25:23Z

crates/core/src/metadata_store/providers/objstore/glue.rs

+            .map_err(|_| WriteError::Internal("Object store fetch channel ".into()))?;
+
+        rx.await.map_err(|_| {
+            WriteError::Internal("Object store fetch channel disconnected".to_string())


The error messages seem to be copied from the get call.

tillrohrmann · 2024-11-22T13:29:43Z

crates/core/src/metadata_store/providers/objstore/immutable_store.rs

+        // serialize as json
+        serde_json::to_vec(self).map(Into::into).map_err(Into::into)


I guess the json encoding would be more helpful if the values were also encoded in json?

crates/core/src/metadata_store/providers/objstore/optimistic_store.rs

tillrohrmann · 2024-11-22T14:37:39Z

crates/core/src/metadata_store/providers/objstore/optimistic_store.rs

+}
+
+#[async_trait::async_trait]
+impl MetadataStore for OptimisticLockingMetadataStore {


Why does the OptimisticLockingMetadataStore implements the MetadataStore?

fixed, it is not any more.

tillrohrmann · 2024-11-22T14:39:01Z

crates/core/src/metadata_store/providers/objstore/optimistic_store.rs

+
+pub struct OptimisticLockingMetadataStore {
+    version_repository: Arc<dyn VersionRepository>,
+    latest_store_cache: ArcSwap<Option<CachedStore>>,


Can this also be a CachedStore (w/o ArcSwap<Option<>>) and change the methods to take a mutable borrow?

If it won't implement the metastore trait then I can do that.
Let me try this out.

worked out beautifully 🧑‍🍳

tillrohrmann · 2024-11-22T14:41:54Z

crates/core/src/metadata_store/providers/objstore/optimistic_store.rs

+                    // refresh and try again
+                    let delay = {
+                        let mut rng = rand::thread_rng();
+                        rng.gen_range(50..300)
+                    };
+                    tokio::time::sleep(Duration::from_millis(delay)).await;


We might be able to replace this with a RetryPolicy.

tillrohrmann · 2024-11-22T14:44:18Z

crates/core/src/metadata_store/providers/objstore/optimistic_store.rs

+
+            match self
+                .version_repository
+                .try_create_version(new_store.current_version(), new_store_bytes)


The correctness of the implementation relies on the fact that our version space won't have any wholes, right? I guess this makes cleaning older versions up a bit more tricky.

Maybe assert that new_store.current_version() == cached_store.store.version().next() to state that there mustn't be any wholes.

pcholakov

I think we might have a better path to implement this, using Object Versioning. A quick search shows that this is supported across MinIO / Azure / GCP, here's the S3 documentation: https://docs.aws.amazon.com/AmazonS3/latest/userguide/Versioning.html.

Here's a sketch of the conditional PUT path:

async fn put(
    &self,
    key: ByteString,
    value: VersionedValue,
    precondition: Precondition,
) -> Result<(), WriteError> {
    let key = object_store::path::Path::from(key.to_string());

    match precondition {
        Precondition::MatchesVersion(version) => {
            // If we have a cached version, we can also add an if-modified-since condition to the GET.
            // Unfortunately we can't set the version explicitly, and a HEAD request doesn't return enough
            // information to determine our own metadata value version.
            let get_result = self.object_store.get(&key).await.map_err(|e| {
                WriteError::Internal(format!("Failed to check precondition: {}", e))
            })?;

            if extract_object_version(&get_result.payload) != version {
                return Err(WriteError::FailedPrecondition(
                    "Version mismatch".to_string(),
                ));
            }

            self.object_store
                .put_opts(
                    &key,
                    PutPayload::from_bytes(serialize_versioned_value(value)),
                    PutOptions::from(PutMode::Update(UpdateVersion::from(&get_result))),
                )
                .await
                .map_err(|e| WriteError::Internal(format!("Failed to update value: {}", e)))?;

            Ok(())
        }
        _ => todo!(),
    }
}

Unfortunately we don't control the versions; in S3 they are a monotonic numbered sequence but we don't get to set them directly - rather, S3 will set them for us. Our own API relies on explicit versions so I'm assuming we'll just serialize them along with the rest of the payload as the object body.

A normal GET request implicitly returns the latest version, no further tricks required. It's also possible to request a particular previous version, but that's not needed for our API. Might be useful for troubleshooting though! (And I'd seriously consider serializing the values as JSON for operations friendliness.)

The only requirement for this all to work is that we must use a bucket with object versioning enabled, which is not the default but is trivial to set. S3 and other stores also support automatic cleanup of old versions using object lifecycle policies, so we don't need to implement that ourselves.

pcholakov · 2024-11-23T02:10:57Z

crates/core/src/metadata_store/providers/objstore/optimistic_store.rs

+            return Ok(());
+        };
+
+        if maybe_cached_version.is_none() || maybe_cached_version.unwrap() < global_latest_version {


I prefer the !is_some_and(...) version personally.

pcholakov · 2024-11-23T02:50:44Z

crates/core/src/metadata_store/providers/objstore/object_store_version_repository.rs

+        let mut stream = self.object_store.list(None);
+
+        // TODO: we should switch to a create internal Version
+        let mut current = 0;
+        while let Some(version) = stream
+            .try_next()
+            .await
+            .map_err(|e| VersionRepositoryError::Network(e.into()))?
+        {
+            if let Some(name) = version.location.filename() {
+                if !name.starts_with("v") {
+                    continue;
+                }
+                if let Ok(v) = name[1..].parse::<u32>() {
+                    if v > current {
+                        current = v;
+                    }
+                }
+            }
+        }


I think we can do this without LIST operations but rather using object versioning.

tillrohrmann · 2024-11-23T10:49:04Z

I think we might have a better path to implement this, using Object Versioning. A quick search shows that this is supported across MinIO / Azure / GCP, here's the S3 documentation: https://docs.aws.amazon.com/AmazonS3/latest/userguide/Versioning.html.

Here's a sketch of the conditional PUT path:
async fn put(
    &self,
    key: ByteString,
    value: VersionedValue,
    precondition: Precondition,
) -> Result<(), WriteError> {
    let key = object_store::path::Path::from(key.to_string());

    match precondition {
        Precondition::MatchesVersion(version) => {
            // If we have a cached version, we can also add an if-modified-since condition to the GET.
            // Unfortunately we can't set the version explicitly, and a HEAD request doesn't return enough
            // information to determine our own metadata value version.
            let get_result = self.object_store.get(&key).await.map_err(|e| {
                WriteError::Internal(format!("Failed to check precondition: {}", e))
            })?;

            if extract_object_version(&get_result.payload) != version {
                return Err(WriteError::FailedPrecondition(
                    "Version mismatch".to_string(),
                ));
            }

            self.object_store
                .put_opts(
                    &key,
                    PutPayload::from_bytes(serialize_versioned_value(value)),
                    PutOptions::from(PutMode::Update(UpdateVersion::from(&get_result))),
                )
                .await
                .map_err(|e| WriteError::Internal(format!("Failed to update value: {}", e)))?;

            Ok(())
        }
        _ => todo!(),
    }
}
Unfortunately we don't control the versions; in S3 they are a monotonic numbered sequence but we don't get to set them directly - rather, S3 will set them for us. Our own API relies on explicit versions so I'm assuming we'll just serialize them along with the rest of the payload as the object body.

A normal GET request implicitly returns the latest version, no further tricks required. It's also possible to request a particular previous version, but that's not needed for our API. Might be useful for troubleshooting though! (And I'd seriously consider serializing the values as JSON for operations friendliness.)

The only requirement for this all to work is that we must use a bucket with object versioning enabled, which is not the default but is trivial to set. S3 and other stores also support automatic cleanup of old versions using object lifecycle policies, so we don't need to implement that ourselves.

Wouldn't the PutMode::Update require an If-Match header support from S3? I thought that it currently only supports If-None-Match natively and for PutMode::Update one would have to use DynamoDB? Or is this different when using versioned buckets?

igalshilman force-pushed the s3metastore branch 5 times, most recently from be3e74c to 638607a Compare November 19, 2024 16:45

tillrohrmann self-requested a review November 20, 2024 14:20

igalshilman force-pushed the s3metastore branch 3 times, most recently from ab8beca to 2f76ae8 Compare November 22, 2024 13:44

tillrohrmann reviewed Nov 22, 2024

View reviewed changes

Add an object_store backed metadata store

7397d7b

igalshilman force-pushed the s3metastore branch from 6d99302 to 7397d7b Compare November 22, 2024 17:20

igalshilman added 2 commits November 22, 2024 18:55

Use cbor for serialization

3722eb6

Remove serde_json

3867ca0

pcholakov reviewed Nov 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[wip] object store metastore #2309

[wip] object store metastore #2309

igalshilman commented Nov 18, 2024

tillrohrmann left a comment •

edited

Loading

tillrohrmann Nov 22, 2024

igalshilman Nov 22, 2024 •

edited

Loading

tillrohrmann Nov 22, 2024

igalshilman Nov 22, 2024

tillrohrmann Nov 22, 2024

tillrohrmann Nov 22, 2024

tillrohrmann Nov 22, 2024

tillrohrmann Nov 22, 2024

igalshilman Nov 22, 2024

tillrohrmann Nov 22, 2024

igalshilman Nov 22, 2024

igalshilman Nov 22, 2024

tillrohrmann Nov 22, 2024

tillrohrmann Nov 22, 2024

tillrohrmann Nov 22, 2024

pcholakov left a comment •

edited

Loading

pcholakov Nov 23, 2024

pcholakov Nov 23, 2024

tillrohrmann commented Nov 23, 2024 •

edited

Loading


		let mut shutdown = pin::pin!(cancellation_watcher());

		let delegate = match builder.build().await {

		// serialize as json
		serde_json::to_vec(self).map(Into::into).map_err(Into::into)

[wip] object store metastore #2309

Are you sure you want to change the base?

[wip] object store metastore #2309

Conversation

igalshilman commented Nov 18, 2024

tillrohrmann left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igalshilman Nov 22, 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

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

pcholakov 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

tillrohrmann commented Nov 23, 2024 • edited Loading

tillrohrmann left a comment •

edited

Loading

igalshilman Nov 22, 2024 •

edited

Loading

pcholakov left a comment •

edited

Loading

tillrohrmann commented Nov 23, 2024 •

edited

Loading