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

feat: remove_via can delete files concurrently #1495

Merged
merged 3 commits into from
Mar 10, 2023
Merged

Conversation

uran0sH
Copy link
Contributor

@uran0sH uran0sH commented Mar 7, 2023

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Delete files concurrently with limit in remove_via.
issue: #1396

src/types/operator/batch.rs Outdated Show resolved Hide resolved
@Xuanwo
Copy link
Member

Xuanwo commented Mar 7, 2023

Please ignore the macos's build, I will fix this today.

@uran0sH
Copy link
Contributor Author

uran0sH commented Mar 7, 2023

Please ignore the macos's build, I will fix this today.

I have no idea how to solve the Memcached test that failed. Could you give me some advice?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 7, 2023

I got it:

/// Delete a key and don't wait for response.
pub async fn delete<K: Display>(&mut self, key: K) -> Result<(), Error> {
    let header = format!("delete {} noreply\r\n", key);
    self.io.write_all(header.as_bytes()).await?;
    self.io.flush().await?;
    Ok(())
}

Seems memcache-async will not wait for the delete reseponse. I think we should report this problem to upstream.

@uran0sH
Copy link
Contributor Author

uran0sH commented Mar 7, 2023

I got it:

/// Delete a key and don't wait for response.
pub async fn delete<K: Display>(&mut self, key: K) -> Result<(), Error> {
    let header = format!("delete {} noreply\r\n", key);
    self.io.write_all(header.as_bytes()).await?;
    self.io.flush().await?;
    Ok(())
}

Seems memcache-async will not wait for the delete reseponse. I think we should report this problem to upstream.

I have reported it to upstream. vavrusa/memcache-async#24

@Xuanwo
Copy link
Member

Xuanwo commented Mar 9, 2023

I have reported it to upstream. vavrusa/memcache-async#24

As there has been no response from memcache-async and its logic is not too complex, I would rather maintain the memcache transport ourselves. Are you willing to port the code from memcache-async to opendal (under MIT license, keep the original license file)?

@uran0sH
Copy link
Contributor Author

uran0sH commented Mar 9, 2023

I have reported it to upstream. vavrusa/memcache-async#24

As there has been no response from memcache-async and its logic is not too complex, I would rather maintain the memcache transport ourselves. Are you willing to port the code from memcache-async to opendal (under MIT license, keep the original license file)?

Ok. So should I copy the file and the license file to opendal? And where should I put these codes?

@Xuanwo
Copy link
Member

Xuanwo commented Mar 9, 2023

Ok. So should I copy the file and the license file to opendal? And where should I put these codes?

Tracked at #1542

@Xuanwo
Copy link
Member

Xuanwo commented Mar 9, 2023

Hi @uran0sH, #1542 has been resolved. You are now able to modify the implementation of delete to better suit your needs.

@uran0sH uran0sH force-pushed the hwy-delete branch 3 times, most recently from d4651d3 to b725103 Compare March 9, 2023 16:05
@Xuanwo
Copy link
Member

Xuanwo commented Mar 9, 2023

Hi there, if the key is not found, OpenDAL should return an Ok response

@uran0sH
Copy link
Contributor Author

uran0sH commented Mar 9, 2023

Hi there, if the key is not found, OpenDAL should return an Ok response

It seems other problems. And I can't reproduce this problem on my machine. My memcached's version is 1.6.14, and I run cargo test memcached --features services-memcached -- --show-output directly.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 9, 2023

It seems other problems. And I can't reproduce this problem on my machine. My memcached's version is 1.6.14, and I run cargo test memcached --features services-memcached -- --show-output directly.

To modify the memcached version utilized during testing, please change

https://github.com/datafuselabs/opendal/blob/a540559515e6e006430d4ff93ad866cd17f4dd24/.github/workflows/service_test_memcached.yml#L42

I think it's worth testing against version 1.6.14, we are finding a memcached's bug? 😱

@Xuanwo
Copy link
Member

Xuanwo commented Mar 10, 2023

Notwithstanding the version of memcached, I will investigate it in detail later.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 10, 2023

Hi, I fixed this issue by remove the noreply in set. The operation could be async internally if we didn't read the reply. Take the logging as an example:

<25 set 110d7750-d750-4acd-a795-16cff427e44a/46582dda-fb8b-4763-9e3b-a784820459ff/98 0 0 13 noreply
>25 NOREPLY STORED
<25 version
>25 VERSION 1.6.18
<25 set 110d7750-d750-4acd-a795-16cff427e44a/46582dda-fb8b-4763-9e3b-a784820459ff/99 0 0 13 noreply
<26 ... <------- 99 didn't got a STORED yet
<26 delete 110d7750-d750-4acd-a795-16cff427e44a/46582dda-fb8b-4763-9e3b-a784820459ff/99
>26 NOT_FOUND  <----------- but the delete op is sent
>25 NOREPLY STORED <--------- 99 is store here!

@Xuanwo
Copy link
Member

Xuanwo commented Mar 10, 2023

Thanks a lot!

@Xuanwo Xuanwo merged commit e9096ef into apache:main Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants