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(services): add hdfs native layout #3933

Merged
merged 27 commits into from
Jan 17, 2024
Merged

Conversation

shbhmrzd
Copy link
Contributor

@shbhmrzd shbhmrzd commented Jan 6, 2024

For #3144

@shbhmrzd
Copy link
Contributor Author

shbhmrzd commented Jan 6, 2024

@Xuanwo
Please review the layout.
Will move ahead with the implementation post that.

core/Cargo.toml Outdated Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
core/Cargo.toml Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
core/src/services/native_hdfs/backend.rs Outdated Show resolved Hide resolved
core/src/services/native_hdfs/docs.md Outdated Show resolved Hide resolved
core/src/services/native_hdfs/lister.rs Outdated Show resolved Hide resolved
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, we can begin our journey now!

core/src/services/hdfs_native/docs.md Outdated Show resolved Hide resolved
@Xuanwo Xuanwo changed the title feat(services): add native_hdfs support feat(services): add hdfs native layout Jan 9, 2024
@Xuanwo
Copy link
Member

Xuanwo commented Jan 10, 2024

Hi, we need to address this error:

error[E0631]: type mismatch in function arguments
   --> core/src/services/hdfs_native/backend.rs:190:22
    |
190 |             .map_err(new_std_io_error)?;
    |              ------- ^^^^^^^^^^^^^^^^ expected due to this
    |              |
    |              required by a bound introduced by this call
    |
   ::: core/src/raw/std_io_util.rs:27:1
    |
27  | pub fn new_std_io_error(err: std::io::Error) -> Error {
    | ----------------------------------------------------- found signature defined here
    |
    = note: expected function signature `fn(hdfs_native::HdfsError) -> _`
               found function signature `fn(std::io::Error) -> _`

We can implement a parse_hdfs_error instead.

@shbhmrzd
Copy link
Contributor Author

Hi, we need to address this error:

error[E0631]: type mismatch in function arguments
   --> core/src/services/hdfs_native/backend.rs:190:22
    |
190 |             .map_err(new_std_io_error)?;
    |              ------- ^^^^^^^^^^^^^^^^ expected due to this
    |              |
    |              required by a bound introduced by this call
    |
   ::: core/src/raw/std_io_util.rs:27:1
    |
27  | pub fn new_std_io_error(err: std::io::Error) -> Error {
    | ----------------------------------------------------- found signature defined here
    |
    = note: expected function signature `fn(hdfs_native::HdfsError) -> _`
               found function signature `fn(std::io::Error) -> _`

We can implement a parse_hdfs_error instead.

Sure will take care of it

@Xuanwo
Copy link
Member

Xuanwo commented Jan 16, 2024

Hi, please make sure previous commits pass the CI before adding 9e34b5f. You can return an unsupported error instead.

@Xuanwo
Copy link
Member

Xuanwo commented Jan 16, 2024

I think it's best to merge the layouts first, then add the detailed implementations.

@shbhmrzd
Copy link
Contributor Author

I think it's best to merge the layouts first, then add the detailed implementations.

Sure, let me revert this change.

@Xuanwo
Copy link
Member

Xuanwo commented Jan 16, 2024

Let's address the CI issues individually, particularly those found at https://github.com/apache/incubator-opendal/actions/runs/7541144383/job/20527241562?pr=3933.

First of all, running cargo fmt to make sure code is formatted.
And than, running cargo clippy --all-targets --features=services-hdfs-native to make sure all newly added code pass the check of clippy.

@Xuanwo Xuanwo closed this Jan 16, 2024
@Xuanwo Xuanwo reopened this Jan 16, 2024
core/Cargo.toml Outdated Show resolved Hide resolved
core/src/raw/hdfs_native_error_util.rs Outdated Show resolved Hide resolved
core/src/raw/mod.rs Outdated Show resolved Hide resolved
@shbhmrzd
Copy link
Contributor Author

Should I push Cargo.lock file as well ?

@Xuanwo
Copy link
Member

Xuanwo commented Jan 16, 2024

Should I push Cargo.lock file as well ?

Yes. Changes to lock file should also be committed.

@Xuanwo Xuanwo marked this pull request as ready for review January 17, 2024 02:01
@Xuanwo Xuanwo requested a review from PsiACE as a code owner January 17, 2024 02:01
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Let's rock!

@Xuanwo Xuanwo merged commit d99cac4 into apache:main Jan 17, 2024
189 checks passed
@shbhmrzd
Copy link
Contributor Author

@Xuanwo Thank you so much for your patience and guidance 🙇

@Xuanwo
Copy link
Member

Xuanwo commented Jan 17, 2024

@Xuanwo Thank you so much for your patience and guidance 🙇

Have fun!

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.

2 participants