Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(core): move move logic from fn metadata(&self) -> Arc<AccessorInfo> to impl<A: Access> Layer<A> for CompleteLayer #4896

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

Lzzzzzt
Copy link
Contributor

@Lzzzzzt Lzzzzzt commented Jul 14, 2024

Which issue does this PR close?

Closes #4888

What changes are included in this PR?

mentioned in #4888

Are there any user-facing changes?

No

@Lzzzzzt Lzzzzzt requested a review from Xuanwo as a code owner July 14, 2024 03:02
@Lzzzzzt Lzzzzzt marked this pull request as draft July 14, 2024 03:35
@@ -382,7 +389,7 @@ impl<A: Access> LayeredAccess for CompleteAccessor<A> {

// Todo: May move the logic to the implement of Layer::layer of CompleteAccessor<A>
fn metadata(&self) -> Arc<AccessorInfo> {
let mut meta = (*self.meta).clone();
let mut meta = self.meta.as_ref().clone();
let cap = meta.full_capability_mut();
if cap.list && cap.write_can_empty {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can remove those code and the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do this, the issue #4888 is not resolved....

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do this, the issue #4888 is not resolved....

Could you elaborate further? I'm not sure which issue remains unresolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can remove those code and the comment?

you means delete the

// Todo: May move the logic to the implement of Layer::layer of CompleteAccessor<A>

and

let mut meta = self.meta.as_ref().clone();

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, since we have created the Arc<AccessInfo> during layer(), we can remove the dup code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's my problem, sry.

@Xuanwo
Copy link
Member

Xuanwo commented Jul 16, 2024

The C/C++ test failed, which is a bit weird. I'll try to figure out what happened. I'm guessing it's related to our Arc been dropped while C still hold it.

@koushiro
Copy link
Contributor

koushiro commented Nov 5, 2024

I'm curious about the C binding error in CI, so I debugged it locally.
The specific reason should be that the changes in this PR have altered the way the CompleteLayer::complete_blocking_create_dir is called, resulting in the opendal_operator_create_dir method in the C binding returning an error.

bindings/c/src/tests/bdd.cpp:

error = opendal_operator_create_dir(this->p, "tmpdir/");  // <====== the error is not nullptr
EXPECT_EQ(error, nullptr);

bindings/c/src/operator.rs

#[no_mangle]
pub unsafe extern "C" fn opendal_operator_create_dir(
    op: &opendal_operator,
    path: *const c_char,
) -> *mut opendal_error {
    assert!(!path.is_null());
    let path = std::ffi::CStr::from_ptr(path)
        .to_str()
        .expect("malformed path");
    if let Err(err) = op.deref().create_dir(path) {
        println!("create dir error: {err}");  // <========
        opendal_error::new(err)
    } else {
        std::ptr::null_mut()
    }
}

core/src/layers/complete.rs

fn complete_blocking_create_dir(&self, path: &str, args: OpCreateDir) -> Result<RpCreateDir> {
    let capability = self.info.full_capability();
    println!(
        "complete_blocking_create_dir: full capability = {:?}",
        capability
    );
    if capability.create_dir && capability.blocking {
        println!("complete_blocking_create_dir: create_dir && blocking");   <===== after this PR
        return self.inner().blocking_create_dir(path, args);
    }
    if capability.write_can_empty && capability.list && capability.blocking {
        println!("complete_blocking_create_dir: write_can_empty && list && blocking");   <====== before this PR
        let (_, mut w) = self.inner.blocking_write(path, OpWrite::default())?;
        oio::BlockingWrite::close(&mut w)?;
        return Ok(RpCreateDir::default());
    }

    Err(self.new_unsupported_error(Operation::BlockingCreateDir))
}

error:

The memory service does not support the blocking_create_dir operation.

[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from OpendalBddTest
[ RUN      ] OpendalBddTest.FeatureTest
complete_blocking_create_dir: full capability = { Stat | Read | Write | CreateDir | Delete | List | Blocking }
complete_blocking_create_dir: create_dir && blocking
create dir error: Unsupported (permanent) at blocking_create_dir, context: { service: memory, path: tmpdir/ } => operation is not supported
/Users/qinxuan/Code/apache/opendal/bindings/c/tests/bdd.cpp:127: Failure
Expected equality of these values:
  error
    Which is: 0x603000004c30
  nullptr
    Which is: (nullptr)

/Users/qinxuan/Code/apache/opendal/bindings/c/tests/bdd.cpp:129: Failure
Expected equality of these values:
  stat.error
    Which is: 0x603000004e10
  nullptr
    Which is: (nullptr)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

idea: move logic from fn metadata(&self) -> Arc<AccessorInfo> to impl<A: Access> Layer<A> for CompleteLayer
3 participants