refactor: Introduce IpfsCore for improved service structure#5950
refactor: Introduce IpfsCore for improved service structure#5950Xuanwo merged 2 commits intoapache:mainfrom
Conversation
dosubot
bot
added
size:L
Behavior tests |
erickguan
left a comment
erickguan
left a comment
There was a problem hiding this comment.
Thank you for getting to IPFS!
Can we use core directly in DirStream?
| impl oio::PageList for DirStream { | ||
| async fn next_page(&self, ctx: &mut oio::PageContext) -> Result<()> { | ||
| let resp = self.backend.ipfs_list(&self.path).await?; | ||
| let resp = self.backend.core.ipfs_list(&self.path).await?; |
There was a problem hiding this comment.
Can we reference self.core directly without going from backend?
Because you have IpfsCore, we can have:
pub struct DirStream {
core: Arc<IpfsCore>,
path: String,
}There was a problem hiding this comment.
Thanks for your review!
DirStream needs some backend functionality
opendal/core/src/services/ipfs/backend.rs
Lines 400 to 404 in 36240c1
or we can have:
pub struct DirStream {
backend: Arc<IpfsBackend>,
core: Arc<IpfsCore>,
path: String,
}There was a problem hiding this comment.
Yes, you're right - we are depending on IpfsBackend::stat. Could we take it a step further and build a function like pub async fn IpfsCore::ipfs_stat(name: &str) -> Result<Metadata>? This would allow you to retrieve metadata for each name directly via the core for DirStream.
As aside, while this change improves the structure, it doesn't resolve the N+1 issue... That is not related to the issue. So we can ignore it for now.
There was a problem hiding this comment.
Could we take it a step further and build a function like pub async fn IpfsCore::ipfs_stat(name: &str) -> Result?
Yes, I'll raise another PR
|
Good plan. Thanks for sharing some love for IPFS! |
There was a problem hiding this comment.
Thank you @miroim for this! And thank you @erickguan for the review.
3 participants
Which issue does this PR close?
Part of #5702
Part of #5677
Rationale for this change
What changes are included in this PR?
IpfsBackendby introducing a newIpfsCorestruct to encapsulate the core functionality and state. The changes aim to improve code organization and maintainability.Are there any user-facing changes?