feat(services/hdfs): refactor HdfsBackend to use HdfsCore for client operations #6033
Conversation
dosubot
bot
added
size:L
|
Hi, seems all hdfs tests are failed: https://github.com/apache/opendal/actions/runs/14482506154/job/40622110318?pr=6033 |
dosubot
bot
added
the
releases-note/feat
|
@Xuanwo yes, just took some time to fix that. The PR is ready for review |
| #[derive(Clone, Debug)] | ||
| pub struct HdfsCore { | ||
| pub info: Arc<AccessorInfo>, | ||
| pub client: Arc<hdrs::Client>, |
There was a problem hiding this comment.
Minor: If we push this further, we should wrap Client's methods instead of exposing the client. I doubt this will be trouble in the future. So either other maintainer's call or you decide the direction.
There was a problem hiding this comment.
Good call. I originally started by abstracting the client calls but then decided against it since the wrappers would be quite thin (mostly wrapping the exact call) and the core library is only used internally within this module. I'm leaning more for letting future changes drive this if there becomes a need to abstract more than just the nested client call.
There was a problem hiding this comment.
Alternatively, we can get a clone of the client and reduce the call chain. I'll add that in a new commit and you can let me know what you think
There was a problem hiding this comment.
I have applied this change here fb2e349. Let me know if it makes sense. Thanks
| Ok(( | ||
| RpDelete::default(), | ||
| oio::OneShotDeleter::new(HdfsDeleter::new(Arc::new(self.clone()))), | ||
| oio::OneShotDeleter::new(HdfsDeleter::new(Arc::clone(&self.core), self.root.clone())), |
There was a problem hiding this comment.
Thank you, @uruemu. We now have a base to start adding code to HdfsCore. We’ll continue iterating to build out this core. Some implementation details can be extracted into functions within the core itself. This will make the backend easier to read and understand, as it will delegate more of the implementation to either the core or the reader/writer.
|
|
||
| fn info(&self) -> Arc<AccessorInfo> { | ||
| self.info.clone() | ||
| self.core.info() |
| pub struct HdfsBackend { | ||
| pub info: Arc<AccessorInfo>, | ||
| pub root: String, | ||
| root: String, |
There was a problem hiding this comment.
Move this into HdfsCore. When using the root in the HdfsBackend, use self.core.root.
| } | ||
|
|
||
| async fn stat(&self, path: &str, _: OpStat) -> Result<RpStat> { | ||
| let p = build_rooted_abs_path(&self.root, path); |
There was a problem hiding this comment.
From this line to 272: m.set_last_modified(meta.modified().into()); This can be a convenience function. Maybe as HdfsCore::hdfs_get_metadata. Then we can use hdfs_get_metadata in these functions:
blocking_statstat
self.core.client.metadata is a good function. We will keep using self.core.client.metadata.
| } | ||
|
|
||
| async fn read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::Reader)> { | ||
| let p = build_rooted_abs_path(&self.root, path); |
There was a problem hiding this comment.
Maybe we can move parts of this implementation to core as HdfsCore::hdfs_read_from_path. Once we have done that, the read function will become:
- read the file
- return results (
HdfsReaderis what OpenDAL returns for users to read data from)
| } | ||
|
|
||
| async fn write(&self, path: &str, op: OpWrite) -> Result<(RpWrite, Self::Writer)> { | ||
| let target_path = build_rooted_abs_path(&self.root, path); |
There was a problem hiding this comment.
Similarly, we can move parts of this implementation to core as HdfsCore::hdfs_open_path_for_writing. Once we have done that, the write function will become:
- open the file for writing
- return results (
HdfsWriteris what OpenDAL returns for users to write data from)
| Ok((RpList::default(), Some(rd))) | ||
| } | ||
|
|
||
| async fn rename(&self, from: &str, to: &str, _args: OpRename) -> Result<RpRename> { |
There was a problem hiding this comment.
I didn't read the details of the implementation. At a glance, this looks similar to blocking_rename. Maybe we can build HdfsCore::hdfs_rename. This will benefit:
renameblocking_rename
| } | ||
|
|
||
| fn blocking_write(&self, path: &str, op: OpWrite) -> Result<(RpWrite, Self::BlockingWriter)> { | ||
| let target_path = build_rooted_abs_path(&self.root, path); |
There was a problem hiding this comment.
We should be able to extract this implementation to core as well.
| } | ||
|
|
||
| fn blocking_read(&self, path: &str, args: OpRead) -> Result<(RpRead, Self::BlockingReader)> { | ||
| let p = build_rooted_abs_path(&self.root, path); |
There was a problem hiding this comment.
We should be able to extract this to core too.
| } | ||
|
|
||
| impl HdfsCore { | ||
| pub fn info(&self) -> Arc<AccessorInfo> { |
There was a problem hiding this comment.
We don't need this function. pub info: Arc<AccessorInfo> is enough.
|
@erickguan Will come back to this PR this week. Sorry, I've been a bit busy |
|
@uruemu Thanks for working on this and your schedule! |
3 participants
Which issue does this PR close?
Addresses #5702: Split service logic to
backendandcoremodulesPart of #5702.
Rationale for this change
What changes are included in this PR?
This pull request refactors the
GridFsBackendby introducing a newGridFsBackendCorestruct to hosting connection functionality and state.GridFsCoreimplements thekv::Adapterstrait, allowing the removal of theAdapterstructAre there any user-facing changes?