feat(services/hdfs_native): Add read,write,list implementation for hdfs_native #4505

shbhmrzd · 2024-04-19T21:29:50Z

For #3144
Kindly review @Xuanwo

Xuanwo · 2024-04-22T03:01:40Z

.github/services/hdfs/hdfs_native/action.yml

This test should be placed in hdfs_native/hdfs.

Xuanwo · 2024-04-22T03:02:00Z

.github/services/hdfs/hdfs_native/action.yml

+        HADOOP_HOME=${HADOOP_HOME}
+        CLASSPATH=${CLASSPATH}
+        LD_LIBRARY_PATH=${JAVA_HOME}/lib/server:${HADOOP_HOME}/lib/native
+        OPENDAL_HDFS_ROOT=/tmp/opendal/


They should be OPENDAL_HDFS_NATIVE_ROOT

Xuanwo · 2024-04-22T03:06:04Z

core/src/services/hdfs_native/lister.rs

        }
    }
 }

 impl oio::List for HdfsNativeLister {
    async fn next(&mut self) -> Result<Option<Entry>> {
-        todo!()
+        let de: FileStatus = match self.lsi.next().await {


use code like the following for better reading:

let Ok(de) = self .lsi .next() .await .transpose() .map_err(parse_hdfs_error)? else { return Ok(None); };

Xuanwo · 2024-04-22T03:06:55Z

core/src/services/hdfs_native/lister.rs

+
+        let entry = if !de.isdir {
+            let odt = DateTime::from_timestamp(de.modification_time as i64, 0);
+            let dt = match odt {


Please don't match an Option if the None case returns an error, use let-else instead.

Xuanwo · 2024-04-22T03:08:26Z

core/src/services/hdfs_native/reader.rs

-        todo!()
+    async fn read_at(&self, offset: u64, limit: usize) -> Result<Buffer> {
+        // Check for offset being too large for usize on 32-bit systems
+        if offset > usize::MAX as u64 {


Please don't run checks in services.

Where can we put this check ?

Where can we put this check ?

We don't need this check for current. It's more like an upstream issue that can't range from i32::MAX..i64::MAX. You can create an opendal issue to track this.

Sure, created issue #4506

Xuanwo · 2024-04-22T03:08:41Z

core/src/services/hdfs_native/reader.rs

+            .f
+            .read_range(offset as usize, limit)
+            .await
+            .map_err(parse_hdfs_error)


Use ? instead of matching an Result.

Xuanwo · 2024-04-22T03:09:20Z

core/src/services/hdfs_native/writer.rs

+        let bytes = bs.to_bytes();
+        let total_bytes = bytes.len();
+        self.f.write(bytes).await.map_err(parse_hdfs_error)?;
+        Ok(total_bytes)


Does self.f.write() make sure that all bytes are written?

Yes, based on the definition, it continues to write until complete buf has been written.

pub async fn write(&mut self, mut buf: Bytes) -> Result<usize> { let bytes_to_write = buf.len(); // Create a shallow copy of the bytes instance to mutate and track what's been read while !buf.is_empty() { let block_writer = self.get_block_writer().await?; block_writer.write(&mut buf).await?; } self.bytes_written += bytes_to_write; Ok(bytes_to_write) }

Please don't rely this behavior. Return written like this:

let n = self.f.write(bytes).await.map_err(parse_hdfs_error)?; Ok(n)

shbhmrzd · 2024-04-22T09:55:51Z

@Xuanwo
Implemented review comments. Please check now.
Could you also please help with the failure in behavior tests? Thank you!

Xuanwo · 2024-04-23T02:01:33Z

.github/scripts/test_behavior/plan.py

@@ -221,9 +221,10 @@ def generate_language_binding_cases(
 ) -> list[dict[str, str]]:
    cases = unique_cases(cases)

-    # Remove hdfs cases for java.
+    # Remove specified services cases for java.


Please don't touch those files, hdfs_native is pure rust which should be fine on other bindings.

Sure
Due to failures in behaviour tests I was trying a bunch of stuff to figure them out. Could you please help with the failures ?

core/src/types/scheme.rs

Xuanwo · 2024-04-23T03:09:06Z

.github/services/hdfs_native/hdfs/action.yml

+        CLASSPATH=${CLASSPATH}
+        LD_LIBRARY_PATH=${JAVA_HOME}/lib/server:${HADOOP_HOME}/lib/native
+        OPENDAL_HDFS_NATIVE_ROOT=/tmp/opendal/
+        OPENDAL_HDFS_NATIVE_URL=http://127.0.0.1:9000


Only hdfs and viewfs schemes are supported

url should be hdfs://127.0.0.1:9000. It's better to have them tested locally before debugging github actions.

Sorry, missed this one. Thank you for pointing this.

Xuanwo · 2024-04-23T03:10:51Z

core/src/services/hdfs_native/writer.rs

+        let bytes = bs.to_bytes();
+        let total_bytes = bytes.len();
+        self.f.write(bytes).await.map_err(parse_hdfs_error)?;
+        Ok(total_bytes)


Please don't rely this behavior. Return written like this:

let n = self.f.write(bytes).await.map_err(parse_hdfs_error)?; Ok(n)

shbhmrzd added 3 commits April 20, 2024 02:50

implementation for hdfs-native read,write.list

8cfcd8b

add behvaviour tests for hdfs_native

5b8bce6

Merge remote-tracking branch 'upstream/main' into native_hdfs_read_write

1a709df

shbhmrzd requested review from Xuanwo and PsiACE as code owners April 19, 2024 21:29

github-actions bot added the releases-note/feat label Apr 19, 2024

github-actions bot requested a review from morristai April 19, 2024 21:30

fix doc build

e9e417b

shbhmrzd force-pushed the native_hdfs_read_write branch 4 times, most recently from 06417f6 to e9e417b Compare April 19, 2024 23:01

Xuanwo reviewed Apr 22, 2024

View reviewed changes

shbhmrzd added 3 commits April 22, 2024 15:22

review comments

af6e959

Merge remote-tracking branch 'upstream/main' into native_hdfs_read_write

42848ea

review comments

a8a6d17

shbhmrzd force-pushed the native_hdfs_read_write branch from 5f3bb5b to a8a6d17 Compare April 22, 2024 10:24

shbhmrzd added 4 commits April 22, 2024 21:59

use url

5b80bec

Merge remote-tracking branch 'upstream/main' into native_hdfs_read_write

75ef053

use localhost url

bcc4aaf

java bindings hdfs-native

c23d576

shbhmrzd requested a review from tisonkun as a code owner April 22, 2024 16:44

shbhmrzd added 2 commits April 22, 2024 22:32

add hdfs-native scheme

76ea03d

restrict node and python bindings for hdfs-native

a274223

shbhmrzd requested review from messense, Zheaoli and suyanhanx as code owners April 22, 2024 17:09

remove hdfs-native from java binding

b3b4435

Xuanwo reviewed Apr 23, 2024

View reviewed changes

revert scheme and test plan changes

77acd20

Xuanwo reviewed Apr 23, 2024

View reviewed changes

shbhmrzd force-pushed the native_hdfs_read_write branch from a7d2563 to 77acd20 Compare April 23, 2024 04:00

shbhmrzd added 7 commits April 23, 2024 09:36

Merge remote-tracking branch 'upstream/main' into native_hdfs_read_write

c3178c0

Merge remote-tracking branch 'upstream/main' into native_hdfs_read_write

fb8965d

update supporting list, read and write

914ead7

add read, write, list capability in accessor info

63c2919

Merge remote-tracking branch 'upstream/main' into native_hdfs_read_write

3346eef

use read instead of read_range as it panics

e795ffb

revert changes

7be3838

tisonkun removed their request for review May 26, 2024 23:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(services/hdfs_native): Add read,write,list implementation for hdfs_native #4505

feat(services/hdfs_native): Add read,write,list implementation for hdfs_native #4505

shbhmrzd commented Apr 19, 2024

Xuanwo Apr 22, 2024

Xuanwo Apr 22, 2024

Xuanwo Apr 22, 2024

Xuanwo Apr 22, 2024

Xuanwo Apr 22, 2024

shbhmrzd Apr 22, 2024

Xuanwo Apr 22, 2024

shbhmrzd Apr 22, 2024

Xuanwo Apr 22, 2024

Xuanwo Apr 22, 2024

shbhmrzd Apr 22, 2024 •

edited

Xuanwo Apr 23, 2024

shbhmrzd commented Apr 22, 2024 •

edited

Xuanwo Apr 23, 2024

shbhmrzd Apr 23, 2024

Xuanwo Apr 23, 2024

shbhmrzd Apr 23, 2024

Xuanwo Apr 23, 2024

feat(services/hdfs_native): Add read,write,list implementation for hdfs_native #4505

Are you sure you want to change the base?

feat(services/hdfs_native): Add read,write,list implementation for hdfs_native #4505

Conversation

shbhmrzd commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shbhmrzd Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shbhmrzd commented Apr 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shbhmrzd Apr 22, 2024 •

edited

shbhmrzd commented Apr 22, 2024 •

edited