You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Berrysoft (via GitHub)" <gi...@apache.org> on 2025/03/18 10:36:12 UTC
[PR] fix(services/compfs): read, delete, copy, rename [opendal]
Berrysoft opened a new pull request, #5807:
URL: https://github.com/apache/opendal/pull/5807
# What changes are included in this PR?
* Recursive path for write, copy, rename: create parent dir before writing.
* Delete: return Ok if not exists.
* Read: fix range calculation logic.
* Lister: follow requirements of the trait.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Berrysoft (via GitHub)" <gi...@apache.org>.
Berrysoft commented on code in PR #5807:
URL: https://github.com/apache/opendal/pull/5807#discussion_r2002315163
##########
core/src/services/compfs/reader.rs:
##########
@@ -47,14 +47,18 @@ impl oio::Read for CompfsReader {
async fn read(&mut self) -> Result<Buffer> {
let pos = self.offset;
if let Some(end) = self.end {
- if end >= pos {
+ if end <= pos {
return Ok(Buffer::new());
}
}
let mut bs = self.core.buf_pool.get();
// reserve 64KB buffer by default, we should allow user to configure this or make it adaptive.
- bs.reserve(64 * 1024);
+ if let Some(end) = self.end {
+ bs.reserve((end - pos) as usize);
+ } else {
+ bs.reserve(64 * 1024);
Review Comment:
Well, maybe it's better to slice it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Berrysoft (via GitHub)" <gi...@apache.org>.
Berrysoft commented on PR #5807:
URL: https://github.com/apache/opendal/pull/5807#issuecomment-2735134998
I have also observed the failure of `test_read_range`, but I don't know the reason.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Berrysoft (via GitHub)" <gi...@apache.org>.
Berrysoft commented on code in PR #5807:
URL: https://github.com/apache/opendal/pull/5807#discussion_r2002350082
##########
core/src/services/compfs/reader.rs:
##########
@@ -47,14 +47,18 @@ impl oio::Read for CompfsReader {
async fn read(&mut self) -> Result<Buffer> {
let pos = self.offset;
if let Some(end) = self.end {
- if end >= pos {
+ if end <= pos {
return Ok(Buffer::new());
}
}
let mut bs = self.core.buf_pool.get();
// reserve 64KB buffer by default, we should allow user to configure this or make it adaptive.
- bs.reserve(64 * 1024);
+ if let Some(end) = self.end {
+ bs.reserve((end - pos) as usize);
+ } else {
+ bs.reserve(64 * 1024);
Review Comment:
Fixed. I think it should work well now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5807:
URL: https://github.com/apache/opendal/pull/5807#issuecomment-2732630954
The only failed tests are:
```
2025-03-18T10:39:49.510162Z ERROR opendal::services: service=compfs name= queued=1 deleted=1: deleter::flush failed Unexpected (temporary) at deleter::flush => directory not empty
Context:
service: compfs
deleted: 1
Source:
Directory not empty (os error 39)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5807:
URL: https://github.com/apache/opendal/pull/5807#discussion_r2002275611
##########
core/src/services/compfs/reader.rs:
##########
@@ -47,14 +47,18 @@ impl oio::Read for CompfsReader {
async fn read(&mut self) -> Result<Buffer> {
let pos = self.offset;
if let Some(end) = self.end {
- if end >= pos {
+ if end < pos {
Review Comment:
Maybe this should be `end <= pos`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5807:
URL: https://github.com/apache/opendal/pull/5807#discussion_r2002312440
##########
core/src/services/compfs/reader.rs:
##########
@@ -47,14 +47,18 @@ impl oio::Read for CompfsReader {
async fn read(&mut self) -> Result<Buffer> {
let pos = self.offset;
if let Some(end) = self.end {
- if end >= pos {
+ if end <= pos {
return Ok(Buffer::new());
}
}
let mut bs = self.core.buf_pool.get();
// reserve 64KB buffer by default, we should allow user to configure this or make it adaptive.
- bs.reserve(64 * 1024);
+ if let Some(end) = self.end {
+ bs.reserve((end - pos) as usize);
+ } else {
+ bs.reserve(64 * 1024);
Review Comment:
`reserve` doesn't guarantee that the buffer size will be exactly `size`; it may end up being larger than expected. Maybe we should call `truncate` to make sure about it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Berrysoft (via GitHub)" <gi...@apache.org>.
Berrysoft commented on code in PR #5807:
URL: https://github.com/apache/opendal/pull/5807#discussion_r2002286792
##########
core/src/services/compfs/reader.rs:
##########
@@ -47,14 +47,18 @@ impl oio::Read for CompfsReader {
async fn read(&mut self) -> Result<Buffer> {
let pos = self.offset;
if let Some(end) = self.end {
- if end >= pos {
+ if end < pos {
Review Comment:
You're right.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5807:
URL: https://github.com/apache/opendal/pull/5807#issuecomment-2735140194
> I have also observed the failure of `test_read_range`, but I don't know the reason.
Can still happen again, will research.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Berrysoft (via GitHub)" <gi...@apache.org>.
Berrysoft commented on code in PR #5807:
URL: https://github.com/apache/opendal/pull/5807#discussion_r2002306567
##########
core/src/services/compfs/reader.rs:
##########
@@ -47,14 +47,18 @@ impl oio::Read for CompfsReader {
async fn read(&mut self) -> Result<Buffer> {
let pos = self.offset;
if let Some(end) = self.end {
- if end >= pos {
+ if end <= pos {
return Ok(Buffer::new());
}
}
let mut bs = self.core.buf_pool.get();
// reserve 64KB buffer by default, we should allow user to configure this or make it adaptive.
- bs.reserve(64 * 1024);
+ if let Some(end) = self.end {
+ bs.reserve((end - pos) as usize);
+ } else {
+ bs.reserve(64 * 1024);
Review Comment:
Yes, if `end` is known, the correct `bs` size is set just as you have pointed out. Line 58.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5807:
URL: https://github.com/apache/opendal/pull/5807#issuecomment-2732648665
The `list` should return itself. For example, list `abc/` should return `abc/` and `abc/xyz`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5807:
URL: https://github.com/apache/opendal/pull/5807#issuecomment-2735115963
Hi, I tested this PR locally and found that `test_read_range` can failed in some cases, happen about 1/4 times:
```rust
---- behavior::test_read_range ----
test panicked: attempt to subtract with overflow
failures:
behavior::test_read_range
thread '<unnamed>' panicked at src/raw/http_util/bytes_range.rs:81:36:
attempt to subtract with overflow
stack backtrace:
0: rust_begin_unwind
at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/std/src/panicking.rs:692:5
1: core::panicking::panic_fmt
at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/core/src/panicking.rs:75:14
2: core::panicking::panic_const::panic_const_sub_overflow
at /rustc/4d91de4e48198da2e33413efdcd9cd2cc0c46688/library/core/src/panicking.rs:178:21
3: opendal::raw::http_util::bytes_range::BytesRange::advance::{{closure}}
at ./src/raw/http_util/bytes_range.rs:81:36
4: core::option::Option<T>::map
at /home/xuanwo/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/option.rs:1119:29
5: opendal::raw::http_util::bytes_range::BytesRange::advance
at ./src/raw/http_util/bytes_range.rs:81:18
6: <opendal::layers::retry::RetryReader<A,<A as opendal::raw::accessor::Access>::Reader> as opendal::raw::oio::read::api::Read>::read::{{closure}}
at ./src/layers/retry.rs:505:21
7: <opendal::layers::retry::RetryWrapper<R,I> as opendal::raw::oio::read::api::Read>::read::{{closure}}::{{closure}}::{{closure}}
at ./src/layers/retry.rs:567:36
8: <backon::retry_with_context::RetryWithContext<B,T,E,Ctx,Fut,FutureFn,SF,RF,NF> as core::future::future::Future>::poll
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/backon-1.4.0/src/retry_with_context.rs:327:45
9: <opendal::layers::retry::RetryWrapper<R,I> as opendal::raw::oio::read::api::Read>::read::{{closure}}
at ./src/layers/retry.rs:576:10
10: <core::pin::Pin<P> as core::future::future::Future>::poll
at /home/xuanwo/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/future/future.rs:124:9
11: <alloc::boxed::Box<T> as opendal::raw::oio::read::api::Read>::read::{{closure}}
at ./src/raw/oio/read/api.rs:113:37
12: <opendal::types::read::buffer_stream::StreamingReader as opendal::raw::oio::read::api::Read>::read::{{closure}}
at ./src/types/read/buffer_stream.rs:62:32
13: <opendal::raw::enum_utils::TwoWays<ONE,TWO> as opendal::raw::oio::read::api::Read>::read::{{closure}}
at ./src/raw/enum_utils.rs:57:41
14: <opendal::types::read::buffer_stream::BufferStream as futures_core::stream::Stream>::poll_next::{{closure}}
at ./src/types/read/buffer_stream.rs:197:49
15: <opendal::types::read::buffer_stream::BufferStream as futures_core::stream::Stream>::poll_next
at ./src/types/read/buffer_stream.rs:204:48
16: <S as futures_core::stream::TryStream>::try_poll_next
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/futures-core-0.3.31/src/stream.rs:206:9
17: <futures_util::stream::try_stream::try_collect::TryCollect<St,C> as core::future::future::Future>::poll
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/futures-util-0.3.31/src/stream/try_stream/try_collect.rs:46:26
18: opendal::types::read::reader::Reader::read::{{closure}}
at ./src/types/read/reader.rs:113:81
19: opendal::types::operator::operator::Operator::read_with::{{closure}}::{{closure}}
at ./src/types/operator/operator.rs:606:52
20: behavior::async_read::test_read_range::{{closure}}
at ./tests/behavior/async_read.rs:106:10
21: tokio::runtime::park::CachedParkThread::block_on::{{closure}}
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/park.rs:281:63
22: tokio::runtime::coop::with_budget
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/coop.rs:107:5
23: tokio::runtime::coop::budget
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/coop.rs:73:5
24: tokio::runtime::park::CachedParkThread::block_on
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/park.rs:281:31
25: tokio::runtime::context::blocking::BlockingRegionGuard::block_on
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/context/blocking.rs:66:9
26: tokio::runtime::handle::Handle::block_on_inner::{{closure}}
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/handle.rs:327:13
27: tokio::runtime::context::runtime::enter_runtime
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/context/runtime.rs:65:16
28: tokio::runtime::handle::Handle::block_on_inner
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/handle.rs:326:9
29: tokio::runtime::handle::Handle::block_on
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/tokio-1.42.0/src/runtime/handle.rs:305:13
30: behavior::utils::build_async_trial::{{closure}}
at ./tests/behavior/utils.rs:71:9
31: libtest_mimic::Trial::test::{{closure}}
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/libtest-mimic-0.8.1/src/lib.rs:119:54
32: core::ops::function::FnOnce::call_once{{vtable.shim}}
at /home/xuanwo/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/ops/function.rs:250:5
33: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
at /home/xuanwo/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/alloc/src/boxed.rs:1993:9
34: libtest_mimic::run_single::{{closure}}
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/libtest-mimic-0.8.1/src/lib.rs:576:43
35: <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
at /home/xuanwo/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/panic/unwind_safe.rs:272:9
36: std::panicking::try::do_call
at /home/xuanwo/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:584:40
37: __rust_try
38: std::panicking::try
at /home/xuanwo/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panicking.rs:547:19
39: std::panic::catch_unwind
at /home/xuanwo/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/std/src/panic.rs:358:14
40: libtest_mimic::run_single
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/libtest-mimic-0.8.1/src/lib.rs:576:5
41: libtest_mimic::run::{{closure}}::{{closure}}
at /home/xuanwo/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/libtest-mimic-0.8.1/src/lib.rs:531:43
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5807:
URL: https://github.com/apache/opendal/pull/5807#discussion_r2002297760
##########
core/src/services/compfs/reader.rs:
##########
@@ -47,14 +47,18 @@ impl oio::Read for CompfsReader {
async fn read(&mut self) -> Result<Buffer> {
let pos = self.offset;
if let Some(end) = self.end {
- if end >= pos {
+ if end <= pos {
return Ok(Buffer::new());
}
}
let mut bs = self.core.buf_pool.get();
// reserve 64KB buffer by default, we should allow user to configure this or make it adaptive.
- bs.reserve(64 * 1024);
+ if let Some(end) = self.end {
+ bs.reserve((end - pos) as usize);
+ } else {
+ bs.reserve(64 * 1024);
Review Comment:
Another issue might be here. If the file size is `1MiB` and we are reading `1024..2048`. We might read too much data than expected.
If `end` is known, we should set the correct `bs` size.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(services/compfs): read, delete, copy, rename [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #5807:
URL: https://github.com/apache/opendal/pull/5807
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org