From 5783b275c549259af30ca2a602ec43949801bab3 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Fri, 6 Dec 2024 01:25:32 +0800 Subject: [PATCH 1/3] refactor: Remove batch concept from opendal Signed-off-by: Xuanwo --- .github/services/s3/r2/disabled_action.yml | 4 +-- bindings/c/include/opendal.h | 14 ---------- bindings/c/src/operator_info.rs | 12 --------- bindings/go/operator_info.go | 12 --------- bindings/go/types.go | 6 ----- bindings/java/src/lib.rs | 5 +--- .../java/org/apache/opendal/Capability.java | 21 --------------- .../apache/opendal/test/OperatorInfoTest.java | 2 -- bindings/java/src/utility.rs | 2 +- bindings/nodejs/generated.d.ts | 6 ----- bindings/nodejs/src/capability.rs | 18 ------------- bindings/python/python/opendal/__init__.pyi | 4 --- bindings/python/src/capability.rs | 10 ------- bindings/ruby/src/capability.rs | 6 ----- core/src/services/gcs/backend.rs | 3 +-- core/src/services/oss/backend.rs | 27 ++++++++++++------- core/src/services/oss/config.rs | 6 +++++ core/src/services/oss/core.rs | 2 +- core/src/services/s3/backend.rs | 23 +++++++++++----- core/src/services/s3/config.rs | 11 ++++++++ core/src/services/s3/core.rs | 2 +- core/src/types/capability.rs | 7 ----- core/src/types/operator/blocking_operator.rs | 21 +++++---------- core/src/types/operator/operator.rs | 20 +++++--------- 24 files changed, 71 insertions(+), 173 deletions(-) diff --git a/.github/services/s3/r2/disabled_action.yml b/.github/services/s3/r2/disabled_action.yml index 892da1fac8f9..52a189cdea38 100644 --- a/.github/services/s3/r2/disabled_action.yml +++ b/.github/services/s3/r2/disabled_action.yml @@ -16,7 +16,7 @@ # under the License. name: r2 -description: 'Behavior test for Cloudflare R2. This service is sponsored by @Xuanwo.' +description: "Behavior test for Cloudflare R2. This service is sponsored by @Xuanwo." runs: using: "composite" @@ -38,6 +38,6 @@ runs: run: | cat << EOF >> $GITHUB_ENV OPENDAL_S3_REGION=auto - OPENDAL_S3_BATCH_MAX_OPERATIONS=700 + OPENDAL_S3_DELETE_MAX_SIZE=700 OPENDAL_S3_DISABLE_STAT_WITH_OVERRIDE=true EOF diff --git a/bindings/c/include/opendal.h b/bindings/c/include/opendal.h index 3560737dda2d..2597edbdbfe9 100644 --- a/bindings/c/include/opendal.h +++ b/bindings/c/include/opendal.h @@ -571,20 +571,6 @@ typedef struct opendal_capability { * If operator supports presign write. */ bool presign_write; - /** - * If operator supports batch. - */ - bool batch; - /** - * If operator supports batch delete. - */ - bool batch_delete; - /** - * The max operations that operator supports in batch. - * - * If it is not set, this will be zero - */ - uintptr_t batch_max_operations; /** * If operator supports shared. */ diff --git a/bindings/c/src/operator_info.rs b/bindings/c/src/operator_info.rs index e08becbc65f3..02a12eb91e54 100644 --- a/bindings/c/src/operator_info.rs +++ b/bindings/c/src/operator_info.rs @@ -126,15 +126,6 @@ pub struct opendal_capability { /// If operator supports presign write. pub presign_write: bool, - /// If operator supports batch. - pub batch: bool, - /// If operator supports batch delete. - pub batch_delete: bool, - /// The max operations that operator supports in batch. - /// - /// If it is not set, this will be zero - pub batch_max_operations: usize, - /// If operator supports shared. pub shared: bool, @@ -263,9 +254,6 @@ impl From for opendal_capability { presign_read: value.presign_read, presign_stat: value.presign_stat, presign_write: value.presign_write, - batch: value.batch, - batch_delete: value.batch_delete, - batch_max_operations: value.batch_max_operations.unwrap_or(0), shared: value.shared, blocking: value.blocking, } diff --git a/bindings/go/operator_info.go b/bindings/go/operator_info.go index 70f5c1e5532e..6ea7f15fcd10 100644 --- a/bindings/go/operator_info.go +++ b/bindings/go/operator_info.go @@ -228,18 +228,6 @@ func (c *Capability) PresignWrite() bool { return c.inner.presignWrite == 1 } -func (c *Capability) Batch() bool { - return c.inner.batch == 1 -} - -func (c *Capability) BatchDelete() bool { - return c.inner.batchDelete == 1 -} - -func (c *Capability) BatchMaxOperations() uint { - return c.inner.batchMaxOperations -} - func (c *Capability) Shared() bool { return c.inner.shared == 1 } diff --git a/bindings/go/types.go b/bindings/go/types.go index 79a4f3fa84e4..a8b56d8bb718 100644 --- a/bindings/go/types.go +++ b/bindings/go/types.go @@ -160,9 +160,6 @@ var ( &ffi.TypeUint8, // presign_read &ffi.TypeUint8, // presign_stat &ffi.TypeUint8, // presign_write - &ffi.TypeUint8, // batch - &ffi.TypeUint8, // batch_delete - &ffi.TypePointer, // batch_max_operations &ffi.TypeUint8, // shared &ffi.TypeUint8, // blocking nil, @@ -202,9 +199,6 @@ type opendalCapability struct { presignRead uint8 presignStat uint8 presignWrite uint8 - batch uint8 - batchDelete uint8 - batchMaxOperations uint shared uint8 blocking uint8 } diff --git a/bindings/java/src/lib.rs b/bindings/java/src/lib.rs index d19da1fb2fb6..eb317f2640a7 100644 --- a/bindings/java/src/lib.rs +++ b/bindings/java/src/lib.rs @@ -94,7 +94,7 @@ fn make_operator_info<'a>(env: &mut JNIEnv<'a>, info: OperatorInfo) -> Result(env: &mut JNIEnv<'a>, cap: Capability) -> Result> { let capability = env.new_object( "org/apache/opendal/Capability", - "(ZZZZZZZZZZZZZZZJJZZZZZZZZZZZZZZJZZ)V", + "(ZZZZZZZZZZZZZZZJJZZZZZZZZZZZZZZ)V", &[ JValue::Bool(cap.stat as jboolean), JValue::Bool(cap.stat_with_if_match as jboolean), @@ -125,9 +125,6 @@ fn make_capability<'a>(env: &mut JNIEnv<'a>, cap: Capability) -> Result bool { - self.0.batch - } - - /// If operator supports batch delete. - #[napi(getter)] - pub fn batch_delete(&self) -> bool { - self.0.batch_delete - } - - /// The max operations that operator supports in batch. - #[napi(getter)] - pub fn batch_max_operations(&self) -> Option { - self.0.batch_max_operations - } - /// If operator supports shared. #[napi(getter)] pub fn shared(&self) -> bool { diff --git a/bindings/python/python/opendal/__init__.pyi b/bindings/python/python/opendal/__init__.pyi index 418d8841a662..6d6645a60bbc 100644 --- a/bindings/python/python/opendal/__init__.pyi +++ b/bindings/python/python/opendal/__init__.pyi @@ -200,9 +200,5 @@ class Capability: presign_stat: bool presign_write: bool - batch: bool - batch_delete: bool - batch_max_operations: Optional[int] - shared: bool blocking: bool diff --git a/bindings/python/src/capability.rs b/bindings/python/src/capability.rs index ac424ba698fc..c1a5423c38c0 100644 --- a/bindings/python/src/capability.rs +++ b/bindings/python/src/capability.rs @@ -98,13 +98,6 @@ pub struct Capability { /// If operator supports presign write. pub presign_write: bool, - /// If operator supports batch. - pub batch: bool, - /// If operator supports batch delete. - pub batch_delete: bool, - /// The max operations that operator supports in batch. - pub batch_max_operations: Option, - /// If operator supports shared. pub shared: bool, @@ -147,9 +140,6 @@ impl Capability { presign_read: capability.presign_read, presign_stat: capability.presign_stat, presign_write: capability.presign_write, - batch: capability.batch, - batch_delete: capability.batch_delete, - batch_max_operations: capability.batch_max_operations, shared: capability.shared, blocking: capability.blocking, } diff --git a/bindings/ruby/src/capability.rs b/bindings/ruby/src/capability.rs index 32166bc0c45d..03116534d57a 100644 --- a/bindings/ruby/src/capability.rs +++ b/bindings/ruby/src/capability.rs @@ -94,9 +94,6 @@ define_accessors!(Capability, { presign_read: bool, presign_stat: bool, presign_write: bool, - batch: bool, - batch_delete: bool, - batch_max_operations: Option, shared: bool, blocking: bool, }); @@ -145,9 +142,6 @@ pub fn include(gem_module: &RModule) -> Result<(), Error> { presign_read, presign_stat, presign_write, - batch, - batch_delete, - batch_max_operations, blocking }); diff --git a/core/src/services/gcs/backend.rs b/core/src/services/gcs/backend.rs index 2b5aac54dcf1..c1227098ff23 100644 --- a/core/src/services/gcs/backend.rs +++ b/core/src/services/gcs/backend.rs @@ -385,6 +385,7 @@ impl Access for GcsBackend { }, delete: true, + delete_max_size: Some(100), copy: true, list: true, @@ -392,8 +393,6 @@ impl Access for GcsBackend { list_with_start_after: true, list_with_recursive: true, - batch: true, - batch_max_operations: Some(100), presign: true, presign_stat: true, presign_read: true, diff --git a/core/src/services/oss/backend.rs b/core/src/services/oss/backend.rs index 0806c3767245..b72eeb1311ec 100644 --- a/core/src/services/oss/backend.rs +++ b/core/src/services/oss/backend.rs @@ -228,8 +228,19 @@ impl OssBuilder { } /// Set maximum batch operations of this backend. - pub fn batch_max_operations(mut self, batch_max_operations: usize) -> Self { - self.config.batch_max_operations = Some(batch_max_operations); + #[deprecated( + since = "0.52.0", + note = "Please use `delete_max_size` instead of `batch_max_operations`" + )] + pub fn batch_max_operations(mut self, delete_max_size: usize) -> Self { + self.config.delete_max_size = Some(delete_max_size); + + self + } + + /// Set maximum delete operations of this backend. + pub fn delete_max_size(mut self, delete_max_size: usize) -> Self { + self.config.delete_max_size = Some(delete_max_size); self } @@ -384,9 +395,9 @@ impl Builder for OssBuilder { let signer = AliyunOssSigner::new(bucket); - let batch_max_operations = self + let delete_max_size = self .config - .batch_max_operations + .delete_max_size .unwrap_or(DEFAULT_BATCH_MAX_OPERATIONS); Ok(OssBackend { @@ -402,7 +413,7 @@ impl Builder for OssBuilder { client, server_side_encryption, server_side_encryption_key_id, - batch_max_operations, + delete_max_size, }), }) } @@ -464,7 +475,8 @@ impl Access for OssBackend { write_with_user_metadata: true, delete: true, - delete_max_size: Some(DEFAULT_BATCH_MAX_OPERATIONS), + delete_max_size: Some(self.core.delete_max_size), + copy: true, list: true, @@ -477,9 +489,6 @@ impl Access for OssBackend { presign_read: true, presign_write: true, - batch: true, - batch_max_operations: Some(self.core.batch_max_operations), - shared: true, ..Default::default() diff --git a/core/src/services/oss/config.rs b/core/src/services/oss/config.rs index a5ebb2d8b31e..73259f3e3105 100644 --- a/core/src/services/oss/config.rs +++ b/core/src/services/oss/config.rs @@ -50,7 +50,13 @@ pub struct OssConfig { /// Access key secret for oss. pub access_key_secret: Option, /// The size of max batch operations. + #[deprecated( + since = "0.52.0", + note = "Please use `delete_max_size` instead of `batch_max_operations`" + )] pub batch_max_operations: Option, + /// The size of max delete operations. + pub delete_max_size: Option, /// If `role_arn` is set, we will use already known config as source /// credential to assume role with `role_arn`. pub role_arn: Option, diff --git a/core/src/services/oss/core.rs b/core/src/services/oss/core.rs index 480ab13987d1..fef9a860496c 100644 --- a/core/src/services/oss/core.rs +++ b/core/src/services/oss/core.rs @@ -75,7 +75,7 @@ pub struct OssCore { pub client: HttpClient, pub loader: AliyunLoader, pub signer: AliyunOssSigner, - pub batch_max_operations: usize, + pub delete_max_size: usize, } impl Debug for OssCore { diff --git a/core/src/services/s3/backend.rs b/core/src/services/s3/backend.rs index 654b28da06ee..52f311415489 100644 --- a/core/src/services/s3/backend.rs +++ b/core/src/services/s3/backend.rs @@ -530,8 +530,19 @@ impl S3Builder { } /// Set maximum batch operations of this backend. + #[deprecated( + since = "0.52.0", + note = "Please use `delete_max_size` instead of `batch_max_operations`" + )] pub fn batch_max_operations(mut self, batch_max_operations: usize) -> Self { - self.config.batch_max_operations = Some(batch_max_operations); + self.config.delete_max_size = Some(batch_max_operations); + + self + } + + /// Set maximum delete operations of this backend. + pub fn delete_max_size(mut self, delete_max_size: usize) -> Self { + self.config.delete_max_size = Some(delete_max_size); self } @@ -858,9 +869,9 @@ impl Builder for S3Builder { let signer = AwsV4Signer::new("s3", ®ion); - let batch_max_operations = self + let delete_max_size = self .config - .batch_max_operations + .delete_max_size .unwrap_or(DEFAULT_BATCH_MAX_OPERATIONS); Ok(S3Backend { @@ -881,8 +892,8 @@ impl Builder for S3Builder { loader, credential_loaded: AtomicBool::new(false), client, - batch_max_operations, checksum_algorithm, + delete_max_size, disable_write_with_if_match: self.config.disable_write_with_if_match, }), }) @@ -950,6 +961,7 @@ impl Access for S3Backend { }, delete: true, + delete_max_size: Some(self.core.delete_max_size), delete_with_version: self.core.enable_versioning, copy: true, @@ -965,9 +977,6 @@ impl Access for S3Backend { presign_read: true, presign_write: true, - batch: true, - batch_max_operations: Some(self.core.batch_max_operations), - shared: true, ..Default::default() diff --git a/core/src/services/s3/config.rs b/core/src/services/s3/config.rs index cbcef7a207d6..e31a70176a8a 100644 --- a/core/src/services/s3/config.rs +++ b/core/src/services/s3/config.rs @@ -162,7 +162,18 @@ pub struct S3Config { /// For example, R2 could return `Internal Error` while batch delete 1000 files. /// /// Please tune this value based on services' document. + #[deprecated( + since = "0.52.0", + note = "Please use `delete_max_size` instead of `batch_max_operations`" + )] pub batch_max_operations: Option, + /// Set hte maximum delete size of this backend. + /// + /// Some compatible services have a limit on the number of operations in a batch request. + /// For example, R2 could return `Internal Error` while batch delete 1000 files. + /// + /// Please tune this value based on services' document. + pub delete_max_size: Option, /// Disable stat with override so that opendal will not send stat request with override queries. /// /// For example, R2 doesn't support stat with `response_content_type` query. diff --git a/core/src/services/s3/core.rs b/core/src/services/s3/core.rs index 6cf689da01f9..b69ef327ff69 100644 --- a/core/src/services/s3/core.rs +++ b/core/src/services/s3/core.rs @@ -96,7 +96,7 @@ pub struct S3Core { pub loader: Box, pub credential_loaded: AtomicBool, pub client: HttpClient, - pub batch_max_operations: usize, + pub delete_max_size: usize, pub checksum_algorithm: Option, pub disable_write_with_if_match: bool, } diff --git a/core/src/types/capability.rs b/core/src/types/capability.rs index f8b87d28a393..2fdf56e87ce3 100644 --- a/core/src/types/capability.rs +++ b/core/src/types/capability.rs @@ -202,13 +202,6 @@ pub struct Capability { /// Indicates if presigned URLs for write operations are supported. pub presign_write: bool, - /// Indicates if batch operations are supported. - pub batch: bool, - /// Indicates if batch delete operations are supported. - pub batch_delete: bool, - /// Maximum number of operations supported in a single batch. - pub batch_max_operations: Option, - /// Indicate if the operator supports shared access. pub shared: bool, diff --git a/core/src/types/operator/blocking_operator.rs b/core/src/types/operator/blocking_operator.rs index 9d7620b15563..1730ead88755 100644 --- a/core/src/types/operator/blocking_operator.rs +++ b/core/src/types/operator/blocking_operator.rs @@ -75,8 +75,6 @@ use crate::*; #[derive(Clone, Debug)] pub struct BlockingOperator { accessor: Accessor, - - limit: usize, } impl BlockingOperator { @@ -89,26 +87,21 @@ impl BlockingOperator { /// # Note /// default batch limit is 1000. pub(crate) fn from_inner(accessor: Accessor) -> Self { - let limit = accessor - .info() - .full_capability() - .batch_max_operations - .unwrap_or(1000); - Self { accessor, limit } + Self { accessor } } /// Get current operator's limit + #[deprecated(note = "limit is no-op for now", since = "0.52.0")] pub fn limit(&self) -> usize { - self.limit + 0 } /// Specify the batch limit. /// /// Default: 1000 - pub fn with_limit(&self, limit: usize) -> Self { - let mut op = self.clone(); - op.limit = limit; - op + #[deprecated(note = "limit is no-op for now", since = "0.52.0")] + pub fn with_limit(&self, _: usize) -> Self { + self.clone() } /// Get information of underlying accessor. @@ -1121,6 +1114,6 @@ impl BlockingOperator { impl From for Operator { fn from(v: BlockingOperator) -> Self { - Operator::from_inner(v.accessor).with_limit(v.limit) + Operator::from_inner(v.accessor) } } diff --git a/core/src/types/operator/operator.rs b/core/src/types/operator/operator.rs index 502d0fa248c0..24bba50d266c 100644 --- a/core/src/types/operator/operator.rs +++ b/core/src/types/operator/operator.rs @@ -62,8 +62,6 @@ pub struct Operator { // accessor is what Operator delegates for accessor: Accessor, - // limit is usually the maximum size of data that operator will handle in one operation - limit: usize, /// The default executor that used to run futures in background. default_executor: Option, } @@ -77,14 +75,8 @@ impl Operator { /// Convert inner accessor into operator. pub fn from_inner(accessor: Accessor) -> Self { - let limit = accessor - .info() - .full_capability() - .batch_max_operations - .unwrap_or(1000); Self { accessor, - limit, default_executor: None, } } @@ -96,17 +88,17 @@ impl Operator { /// Get current operator's limit. /// Limit is usually the maximum size of data that operator will handle in one operation. + #[deprecated(note = "limit is no-op for now", since = "0.52.0")] pub fn limit(&self) -> usize { - self.limit + 0 } /// Specify the batch limit. /// /// Default: 1000 - pub fn with_limit(&self, limit: usize) -> Self { - let mut op = self.clone(); - op.limit = limit; - op + #[deprecated(note = "limit is no-op for now", since = "0.52.0")] + pub fn with_limit(&self, _: usize) -> Self { + self.clone() } /// Get the default executor. @@ -143,7 +135,7 @@ impl Operator { /// /// This operation is nearly no cost. pub fn blocking(&self) -> BlockingOperator { - BlockingOperator::from_inner(self.accessor.clone()).with_limit(self.limit) + BlockingOperator::from_inner(self.accessor.clone()) } } From ab4f05972cb5649c9ec459bfcee1e5077f20c968 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Fri, 6 Dec 2024 01:32:40 +0800 Subject: [PATCH 2/3] Fix java Signed-off-by: Xuanwo --- bindings/java/src/utility.rs | 2 +- core/src/services/s3/config.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/bindings/java/src/utility.rs b/bindings/java/src/utility.rs index d04a11eeaf2b..1a57a384d8e8 100644 --- a/bindings/java/src/utility.rs +++ b/bindings/java/src/utility.rs @@ -16,7 +16,7 @@ // under the License. use jni::objects::JClass; -use jni::objects::JObject; w +use jni::objects::JObject; use jni::sys::jobjectArray; use jni::sys::jsize; use jni::JNIEnv; diff --git a/core/src/services/s3/config.rs b/core/src/services/s3/config.rs index e31a70176a8a..41899e9e4b90 100644 --- a/core/src/services/s3/config.rs +++ b/core/src/services/s3/config.rs @@ -167,7 +167,7 @@ pub struct S3Config { note = "Please use `delete_max_size` instead of `batch_max_operations`" )] pub batch_max_operations: Option, - /// Set hte maximum delete size of this backend. + /// Set the maximum delete size of this backend. /// /// Some compatible services have a limit on the number of operations in a batch request. /// For example, R2 could return `Internal Error` while batch delete 1000 files. From dc5fb7c4a26a4c99461a3b69e023ae1b3ca4b9e5 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Fri, 6 Dec 2024 13:31:27 +0800 Subject: [PATCH 3/3] Fix size Signed-off-by: Xuanwo --- core/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/lib.rs b/core/src/lib.rs index 2eca7309a5fa..7a7f6eae18ff 100644 --- a/core/src/lib.rs +++ b/core/src/lib.rs @@ -151,7 +151,7 @@ mod tests { /// unexpected struct/enum size change. #[test] fn assert_size() { - assert_eq!(40, size_of::()); + assert_eq!(32, size_of::()); assert_eq!(296, size_of::()); assert_eq!(272, size_of::()); assert_eq!(1, size_of::());