From f19bb36e42600a12252355d36c4b511fb2914e8e Mon Sep 17 00:00:00 2001 From: Lachlan Deakin Date: Wed, 3 Jan 2024 10:53:33 +1100 Subject: [PATCH] Storage and array erase methods now return `()` instead of `bool` --- CHANGELOG.md | 1 + src/array/array_async_writable.rs | 4 ++-- src/array/array_sync_writable.rs | 4 ++-- src/storage/storage_async.rs | 23 ++++++++----------- src/storage/storage_handle.rs | 12 +++++----- src/storage/storage_sync.rs | 21 ++++++++--------- .../performance_metrics.rs | 12 +++++----- src/storage/storage_transformer/usage_log.rs | 12 +++++----- src/storage/store/store_async.rs | 1 + src/storage/store/store_async/object_store.rs | 9 ++++---- src/storage/store/store_async/opendal.rs | 10 ++++---- src/storage/store/store_sync.rs | 1 + .../store/store_sync/filesystem_store.rs | 18 +++++++++++---- src/storage/store/store_sync/memory_store.rs | 11 ++++----- src/storage/store/store_sync/opendal.rs | 10 ++++---- 15 files changed, 75 insertions(+), 74 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bea425e0..bbeacc27 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Separate `Array` methods into separate files for each storage trait - **Breaking**: Remove `_opt` and `par_` variants of `async_retrieve_array_subset` and `async_store_array_subset` (including `_elements` and `_ndarray` variants) - Revise `array_write_read` and `async_array_write_read` examples + - **Breaking**: Storage `erase`/`erase_values`/`erase_prefix` methods and `Array::erase_chunk` now return `()` instead of `bool` and succeed irrespective of the whether the key/prefix exists ## [0.8.0] - 2023-12-26 diff --git a/src/array/array_async_writable.rs b/src/array/array_async_writable.rs index 2712f44f..d7a7ea87 100644 --- a/src/array/array_async_writable.rs +++ b/src/array/array_async_writable.rs @@ -231,11 +231,11 @@ impl Array { /// Erase the chunk at `chunk_indices`. /// - /// Returns true if the chunk was erased, or false if it did not exist. + /// Succeeds if the key does not exist. /// /// # Errors /// Returns a [`StorageError`] if there is an underlying store error. - pub async fn async_erase_chunk(&self, chunk_indices: &[u64]) -> Result { + pub async fn async_erase_chunk(&self, chunk_indices: &[u64]) -> Result<(), StorageError> { let storage_handle = Arc::new(StorageHandle::new(&*self.storage)); let storage_transformer = self .storage_transformers() diff --git a/src/array/array_sync_writable.rs b/src/array/array_sync_writable.rs index ee760e33..b86c7c26 100644 --- a/src/array/array_sync_writable.rs +++ b/src/array/array_sync_writable.rs @@ -313,11 +313,11 @@ impl Array { /// Erase the chunk at `chunk_indices`. /// - /// Returns true if the chunk was erased, or false if it did not exist. + /// Succeeds if the chunk does not exist. /// /// # Errors /// Returns a [`StorageError`] if there is an underlying store error. - pub fn erase_chunk(&self, chunk_indices: &[u64]) -> Result { + pub fn erase_chunk(&self, chunk_indices: &[u64]) -> Result<(), StorageError> { let storage_handle = Arc::new(StorageHandle::new(&*self.storage)); let storage_transformer = self .storage_transformers() diff --git a/src/storage/storage_async.rs b/src/storage/storage_async.rs index fde45e2c..5c3402f8 100644 --- a/src/storage/storage_async.rs +++ b/src/storage/storage_async.rs @@ -234,35 +234,30 @@ pub trait AsyncWritableStorageTraits: Send + Sync { /// Erase a [`StoreKey`]. /// - /// Returns true if the key exists and was erased, or false if the key does not exist. + /// Succeeds if the key does not exist. /// /// # Errors /// Returns a [`StorageError`] if there is an underlying storage error. - async fn erase(&self, key: &StoreKey) -> Result; + async fn erase(&self, key: &StoreKey) -> Result<(), StorageError>; /// Erase a list of [`StoreKey`]. /// - /// Returns true if all keys existed and were erased, or false if any key does not exist. - /// /// # Errors /// Returns a [`StorageError`] if there is an underlying storage error. - async fn erase_values(&self, keys: &[StoreKey]) -> Result { + async fn erase_values(&self, keys: &[StoreKey]) -> Result<(), StorageError> { let futures_erase = keys.iter().map(|key| self.erase(key)); - let result = futures::future::join_all(futures_erase) + futures::future::join_all(futures_erase) .await .into_iter() .collect::, _>>()?; - let all_deleted = result.iter().all(|b| *b); - Ok(all_deleted) + Ok(()) } /// Erase all [`StoreKey`] under [`StorePrefix`]. /// - /// Returns true if the prefix and all its children were removed. - /// /// # Errors - /// Returns a [`StorageError`] is the prefix is not in the store, or the erase otherwise fails. - async fn erase_prefix(&self, prefix: &StorePrefix) -> Result; + /// Returns a [`StorageError`] if there is an underlying storage error. + async fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError>; } /// A supertrait of [`AsyncReadableStorageTraits`] and [`AsyncWritableStorageTraits`]. @@ -396,7 +391,7 @@ pub async fn async_erase_chunk( array_path: &NodePath, chunk_grid_indices: &[u64], chunk_key_encoding: &ChunkKeyEncoding, -) -> Result { +) -> Result<(), StorageError> { storage .erase(&data_key( array_path, @@ -469,7 +464,7 @@ pub async fn async_discover_nodes( pub async fn async_erase_node( storage: &dyn AsyncWritableStorageTraits, path: &NodePath, -) -> Result { +) -> Result<(), StorageError> { let prefix = path.try_into()?; storage.erase_prefix(&prefix).await } diff --git a/src/storage/storage_handle.rs b/src/storage/storage_handle.rs index 8da0e9eb..73662e2e 100644 --- a/src/storage/storage_handle.rs +++ b/src/storage/storage_handle.rs @@ -95,15 +95,15 @@ impl WritableStorageTraits self.0.set_partial_values(key_start_values) } - fn erase(&self, key: &super::StoreKey) -> Result { + fn erase(&self, key: &super::StoreKey) -> Result<(), super::StorageError> { self.0.erase(key) } - fn erase_values(&self, keys: &[super::StoreKey]) -> Result { + fn erase_values(&self, keys: &[super::StoreKey]) -> Result<(), super::StorageError> { self.0.erase_values(keys) } - fn erase_prefix(&self, prefix: &super::StorePrefix) -> Result { + fn erase_prefix(&self, prefix: &super::StorePrefix) -> Result<(), super::StorageError> { self.0.erase_prefix(prefix) } } @@ -193,15 +193,15 @@ impl AsyncWritableStorageTraits self.0.set_partial_values(key_start_values).await } - async fn erase(&self, key: &super::StoreKey) -> Result { + async fn erase(&self, key: &super::StoreKey) -> Result<(), super::StorageError> { self.0.erase(key).await } - async fn erase_values(&self, keys: &[super::StoreKey]) -> Result { + async fn erase_values(&self, keys: &[super::StoreKey]) -> Result<(), super::StorageError> { self.0.erase_values(keys).await } - async fn erase_prefix(&self, prefix: &super::StorePrefix) -> Result { + async fn erase_prefix(&self, prefix: &super::StorePrefix) -> Result<(), super::StorageError> { self.0.erase_prefix(prefix).await } } diff --git a/src/storage/storage_sync.rs b/src/storage/storage_sync.rs index af02d16f..5abd1c8f 100644 --- a/src/storage/storage_sync.rs +++ b/src/storage/storage_sync.rs @@ -226,7 +226,7 @@ pub trait WritableStorageTraits: Send + Sync { /// # Errors /// /// Returns a [`StorageError`] if there is an underlying storage error. - fn erase(&self, key: &StoreKey) -> Result; + fn erase(&self, key: &StoreKey) -> Result<(), StorageError>; /// Erase a list of [`StoreKey`]. /// @@ -235,12 +235,9 @@ pub trait WritableStorageTraits: Send + Sync { /// # Errors /// /// Returns a [`StorageError`] if there is an underlying storage error. - fn erase_values(&self, keys: &[StoreKey]) -> Result { - let mut all_deleted = true; - for key in keys { - all_deleted = all_deleted && self.erase(key)?; - } - Ok(all_deleted) + fn erase_values(&self, keys: &[StoreKey]) -> Result<(), StorageError> { + keys.iter().try_for_each(|key| self.erase(key))?; + Ok(()) } /// Erase all [`StoreKey`] under [`StorePrefix`]. @@ -249,7 +246,7 @@ pub trait WritableStorageTraits: Send + Sync { /// /// # Errors /// Returns a [`StorageError`] is the prefix is not in the store, or the erase otherwise fails. - fn erase_prefix(&self, prefix: &StorePrefix) -> Result; + fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError>; } /// A supertrait of [`ReadableStorageTraits`] and [`WritableStorageTraits`]. @@ -359,6 +356,8 @@ pub fn retrieve_chunk( /// Erase a chunk. /// +/// Succeeds if the chunk does not exist. +/// /// # Errors /// Returns a [`StorageError`] if there is an underlying error with the store. pub fn erase_chunk( @@ -366,7 +365,7 @@ pub fn erase_chunk( array_path: &NodePath, chunk_grid_indices: &[u64], chunk_key_encoding: &ChunkKeyEncoding, -) -> Result { +) -> Result<(), StorageError> { storage.erase(&data_key( array_path, chunk_grid_indices, @@ -425,14 +424,14 @@ pub fn discover_nodes(storage: &dyn ListableStorageTraits) -> Result Result { +) -> Result<(), StorageError> { let prefix = path.try_into()?; storage.erase_prefix(&prefix) } diff --git a/src/storage/storage_transformer/performance_metrics.rs b/src/storage/storage_transformer/performance_metrics.rs index 57affb66..ef14cccc 100644 --- a/src/storage/storage_transformer/performance_metrics.rs +++ b/src/storage/storage_transformer/performance_metrics.rs @@ -269,15 +269,15 @@ impl WritableStorageTraits self.storage.set_partial_values(key_start_values) } - fn erase(&self, key: &StoreKey) -> Result { + fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { self.storage.erase(key) } - fn erase_values(&self, keys: &[StoreKey]) -> Result { + fn erase_values(&self, keys: &[StoreKey]) -> Result<(), StorageError> { self.storage.erase_values(keys) } - fn erase_prefix(&self, prefix: &StorePrefix) -> Result { + fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { self.storage.erase_prefix(prefix) } } @@ -408,15 +408,15 @@ impl AsyncWritableStorageTraits self.storage.set_partial_values(key_start_values).await } - async fn erase(&self, key: &StoreKey) -> Result { + async fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { self.storage.erase(key).await } - async fn erase_values(&self, keys: &[StoreKey]) -> Result { + async fn erase_values(&self, keys: &[StoreKey]) -> Result<(), StorageError> { self.storage.erase_values(keys).await } - async fn erase_prefix(&self, prefix: &StorePrefix) -> Result { + async fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { self.storage.erase_prefix(prefix).await } } diff --git a/src/storage/storage_transformer/usage_log.rs b/src/storage/storage_transformer/usage_log.rs index cbc8c395..173c5f30 100644 --- a/src/storage/storage_transformer/usage_log.rs +++ b/src/storage/storage_transformer/usage_log.rs @@ -261,7 +261,7 @@ impl WritableStorageTraits self.storage.set_partial_values(key_start_values) } - fn erase(&self, key: &StoreKey) -> Result { + fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { writeln!( self.handle.lock().unwrap(), "{}erase({key:?}", @@ -270,7 +270,7 @@ impl WritableStorageTraits self.storage.erase(key) } - fn erase_values(&self, keys: &[StoreKey]) -> Result { + fn erase_values(&self, keys: &[StoreKey]) -> Result<(), StorageError> { writeln!( self.handle.lock().unwrap(), "{}erase_values({keys:?}", @@ -279,7 +279,7 @@ impl WritableStorageTraits self.storage.erase_values(keys) } - fn erase_prefix(&self, prefix: &StorePrefix) -> Result { + fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { writeln!( self.handle.lock().unwrap(), "{}erase_prefix({prefix:?}", @@ -436,7 +436,7 @@ impl AsyncWritableStorageTraits self.storage.set_partial_values(key_start_values).await } - async fn erase(&self, key: &StoreKey) -> Result { + async fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { writeln!( self.handle.lock().unwrap(), "{}erase({key:?}", @@ -445,7 +445,7 @@ impl AsyncWritableStorageTraits self.storage.erase(key).await } - async fn erase_values(&self, keys: &[StoreKey]) -> Result { + async fn erase_values(&self, keys: &[StoreKey]) -> Result<(), StorageError> { writeln!( self.handle.lock().unwrap(), "{}erase_values({keys:?}", @@ -454,7 +454,7 @@ impl AsyncWritableStorageTraits self.storage.erase_values(keys).await } - async fn erase_prefix(&self, prefix: &StorePrefix) -> Result { + async fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { writeln!( self.handle.lock().unwrap(), "{}erase_prefix({prefix:?}", diff --git a/src/storage/store/store_async.rs b/src/storage/store/store_async.rs index 326f7507..ee4c7f00 100644 --- a/src/storage/store/store_async.rs +++ b/src/storage/store/store_async.rs @@ -46,6 +46,7 @@ mod test_util { store.set(&"erase".try_into()?, &[]).await?; store.erase(&"erase".try_into()?).await?; + store.erase(&"erase".try_into()?).await?; // succeeds store.set(&"erase_values_0".try_into()?, &[]).await?; store.set(&"erase_values_1".try_into()?, &[]).await?; diff --git a/src/storage/store/store_async/object_store.rs b/src/storage/store/store_async/object_store.rs index cf880d39..41108720 100644 --- a/src/storage/store/store_async/object_store.rs +++ b/src/storage/store/store_async/object_store.rs @@ -156,11 +156,12 @@ impl AsyncWritableStorageTraits for AsyncObjectSto crate::storage::async_store_set_partial_values(self, key_start_values).await } - async fn erase(&self, key: &StoreKey) -> Result { - Ok(handle_result(self.object_store.delete(&key_to_path(key)).await)?.is_some()) + async fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { + handle_result(self.object_store.delete(&key_to_path(key)).await)?; + Ok(()) } - async fn erase_prefix(&self, prefix: &StorePrefix) -> Result { + async fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { let prefix: object_store::path::Path = prefix.as_str().into(); let locations = self .object_store @@ -171,7 +172,7 @@ impl AsyncWritableStorageTraits for AsyncObjectSto .delete_stream(locations) .try_collect::>() .await?; - Ok(true) + Ok(()) } } diff --git a/src/storage/store/store_async/opendal.rs b/src/storage/store/store_async/opendal.rs index 5714bc6b..5befcce1 100644 --- a/src/storage/store/store_async/opendal.rs +++ b/src/storage/store/store_async/opendal.rs @@ -152,16 +152,14 @@ impl AsyncWritableStorageTraits for AsyncOpendalStore { crate::storage::async_store_set_partial_values(self, key_start_values).await } - async fn erase(&self, key: &StoreKey) -> Result { - // FIXME: Make erase return ()? + async fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { self.operator.remove(vec![key.to_string()]).await?; - Ok(true) + Ok(()) } - async fn erase_prefix(&self, prefix: &StorePrefix) -> Result { - // FIXME: Make erase_prefix return ()? + async fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { self.operator.remove_all(prefix.as_str()).await?; - Ok(true) + Ok(()) } } diff --git a/src/storage/store/store_sync.rs b/src/storage/store/store_sync.rs index ec1debbc..57e499b0 100644 --- a/src/storage/store/store_sync.rs +++ b/src/storage/store/store_sync.rs @@ -45,6 +45,7 @@ mod test_util { store.set(&"erase".try_into()?, &[])?; store.erase(&"erase".try_into()?)?; + store.erase(&"erase".try_into()?)?; // succeeds store.set(&"erase_values_0".try_into()?, &[])?; store.set(&"erase_values_1".try_into()?, &[])?; diff --git a/src/storage/store/store_sync/filesystem_store.rs b/src/storage/store/store_sync/filesystem_store.rs index 3b920809..37c53018 100644 --- a/src/storage/store/store_sync/filesystem_store.rs +++ b/src/storage/store/store_sync/filesystem_store.rs @@ -299,7 +299,7 @@ impl WritableStorageTraits for FilesystemStore { store_set_partial_values(self, key_start_values) } - fn erase(&self, key: &StoreKey) -> Result { + fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { if self.readonly { return Err(StorageError::ReadOnly); } @@ -308,10 +308,18 @@ impl WritableStorageTraits for FilesystemStore { let _lock = file.write(); let key_path = self.key_to_fspath(key); - Ok(std::fs::remove_file(key_path).is_ok()) + let result = std::fs::remove_file(key_path); + if let Err(err) = result { + match err.kind() { + std::io::ErrorKind::NotFound => Ok(()), + _ => Err(err.into()), + } + } else { + Ok(()) + } } - fn erase_prefix(&self, prefix: &StorePrefix) -> Result { + fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { if self.readonly { return Err(StorageError::ReadOnly); } @@ -322,11 +330,11 @@ impl WritableStorageTraits for FilesystemStore { let result = std::fs::remove_dir_all(prefix_path); if let Err(err) = result { match err.kind() { - std::io::ErrorKind::NotFound => Ok(false), + std::io::ErrorKind::NotFound => Ok(()), _ => Err(err.into()), } } else { - Ok(true) + Ok(()) } } } diff --git a/src/storage/store/store_sync/memory_store.rs b/src/storage/store/store_sync/memory_store.rs index 7b9706bf..8b6dc899 100644 --- a/src/storage/store/store_sync/memory_store.rs +++ b/src/storage/store/store_sync/memory_store.rs @@ -151,22 +151,21 @@ impl WritableStorageTraits for MemoryStore { store_set_partial_values(self, key_start_values) } - fn erase(&self, key: &StoreKey) -> Result { + fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { let mut data_map = self.data_map.lock().unwrap(); - Ok(data_map.remove(key).is_some()) + data_map.remove(key); + Ok(()) } - fn erase_prefix(&self, prefix: &StorePrefix) -> Result { + fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { let mut data_map = self.data_map.lock().unwrap(); let keys: Vec = data_map.keys().cloned().collect(); - let mut any_deletions = false; for key in keys { if key.has_prefix(prefix) { data_map.remove(&key); - any_deletions = true; } } - Ok(any_deletions) + Ok(()) } } diff --git a/src/storage/store/store_sync/opendal.rs b/src/storage/store/store_sync/opendal.rs index d676800c..3d7f4679 100644 --- a/src/storage/store/store_sync/opendal.rs +++ b/src/storage/store/store_sync/opendal.rs @@ -156,16 +156,14 @@ impl WritableStorageTraits for OpendalStore { crate::storage::store_set_partial_values(self, key_start_values) } - fn erase(&self, key: &StoreKey) -> Result { - // FIXME: Make erase return ()? + fn erase(&self, key: &StoreKey) -> Result<(), StorageError> { self.operator.remove(vec![key.to_string()])?; - Ok(true) + Ok(()) } - fn erase_prefix(&self, prefix: &StorePrefix) -> Result { - // FIXME: Make erase_prefix return ()? + fn erase_prefix(&self, prefix: &StorePrefix) -> Result<(), StorageError> { self.operator.remove_all(prefix.as_str())?; - Ok(true) + Ok(()) } }