You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "shbhmrzd (via GitHub)" <gi...@apache.org> on 2024/04/19 21:29:52 UTC
[PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
shbhmrzd opened a new pull request, #4505:
URL: https://github.com/apache/opendal/pull/4505
For #3144
Kindly review @Xuanwo
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1574069985
##########
core/src/services/hdfs_native/reader.rs:
##########
@@ -18,20 +18,40 @@
use hdfs_native::file::FileReader;
use crate::raw::*;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeReader {
- _f: FileReader,
+ f: FileReader,
}
impl HdfsNativeReader {
pub fn new(f: FileReader) -> Self {
- HdfsNativeReader { _f: f }
+ HdfsNativeReader { f }
}
}
impl oio::Read for HdfsNativeReader {
- async fn read_at(&self, _offset: u64, _limit: usize) -> Result<Buffer> {
- 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 {
+ return Err(Error::new(
+ ErrorKind::InvalidInput,
+ "Offset is too large for this platform",
+ ));
+ }
+
+ // Perform the read operation using read_range
+ let bytes = match self
+ .f
+ .read_range(offset as usize, limit)
+ .await
+ .map_err(parse_hdfs_error)
Review Comment:
Use `?` instead of matching an Result.
##########
core/src/services/hdfs_native/lister.rs:
##########
@@ -15,28 +15,62 @@
// specific language governing permissions and limitations
// under the License.
-use std::sync::Arc;
+use chrono::DateTime;
+use hdfs_native::client::{FileStatus, ListStatusIterator};
-use crate::raw::oio;
use crate::raw::oio::Entry;
+use crate::raw::{build_rel_path, oio};
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeLister {
- _path: String,
- _client: Arc<hdfs_native::Client>,
+ root: String,
+ lsi: ListStatusIterator,
}
impl HdfsNativeLister {
- pub fn new(path: String, client: Arc<hdfs_native::Client>) -> Self {
- HdfsNativeLister {
- _path: path,
- _client: client,
+ pub fn new(root: &str, lsi: ListStatusIterator) -> Self {
+ Self {
+ root: root.to_string(),
+ lsi,
}
}
}
impl oio::List for HdfsNativeLister {
async fn next(&mut self) -> Result<Option<Entry>> {
- todo!()
+ let de: FileStatus = match self.lsi.next().await {
Review Comment:
use code like the following for better reading:
```rust
let Ok(de) = self
.lsi
.next()
.await
.transpose()
.map_err(parse_hdfs_error)?
else {
return Ok(None);
};
```
##########
core/src/services/hdfs_native/reader.rs:
##########
@@ -18,20 +18,40 @@
use hdfs_native::file::FileReader;
use crate::raw::*;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeReader {
- _f: FileReader,
+ f: FileReader,
}
impl HdfsNativeReader {
pub fn new(f: FileReader) -> Self {
- HdfsNativeReader { _f: f }
+ HdfsNativeReader { f }
}
}
impl oio::Read for HdfsNativeReader {
- async fn read_at(&self, _offset: u64, _limit: usize) -> Result<Buffer> {
- 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 {
Review Comment:
Please don't run checks in services.
##########
core/src/services/hdfs_native/lister.rs:
##########
@@ -15,28 +15,62 @@
// specific language governing permissions and limitations
// under the License.
-use std::sync::Arc;
+use chrono::DateTime;
+use hdfs_native::client::{FileStatus, ListStatusIterator};
-use crate::raw::oio;
use crate::raw::oio::Entry;
+use crate::raw::{build_rel_path, oio};
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeLister {
- _path: String,
- _client: Arc<hdfs_native::Client>,
+ root: String,
+ lsi: ListStatusIterator,
}
impl HdfsNativeLister {
- pub fn new(path: String, client: Arc<hdfs_native::Client>) -> Self {
- HdfsNativeLister {
- _path: path,
- _client: client,
+ pub fn new(root: &str, lsi: ListStatusIterator) -> Self {
+ Self {
+ root: root.to_string(),
+ lsi,
}
}
}
impl oio::List for HdfsNativeLister {
async fn next(&mut self) -> Result<Option<Entry>> {
- todo!()
+ let de: FileStatus = match self.lsi.next().await {
+ Some(res) => match res {
+ Ok(fs) => fs,
+ Err(e) => return Err(parse_hdfs_error(e)),
+ },
+ None => return Ok(None),
+ };
+
+ let path = build_rel_path(&self.root, &de.path);
+
+ let entry = if !de.isdir {
+ let odt = DateTime::from_timestamp(de.modification_time as i64, 0);
+ let dt = match odt {
Review Comment:
Please don't `match` an `Option` if the `None` case returns an error, use `let-else` instead.
##########
.github/services/hdfs/hdfs_native/action.yml:
##########
Review Comment:
This test should be placed in `hdfs_native/hdfs`.
##########
core/src/services/hdfs_native/writer.rs:
##########
@@ -18,25 +18,29 @@
use hdfs_native::file::FileWriter;
use crate::raw::oio;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeWriter {
- _f: FileWriter,
+ f: FileWriter,
}
impl HdfsNativeWriter {
pub fn new(f: FileWriter) -> Self {
- HdfsNativeWriter { _f: f }
+ HdfsNativeWriter { f }
}
}
impl oio::Write for HdfsNativeWriter {
- async fn write(&mut self, _bs: Buffer) -> Result<usize> {
- todo!()
+ async fn write(&mut self, bs: Buffer) -> Result<usize> {
+ let bytes = bs.to_bytes();
+ let total_bytes = bytes.len();
+ self.f.write(bytes).await.map_err(parse_hdfs_error)?;
+ Ok(total_bytes)
Review Comment:
Does `self.f.write()` make sure that all bytes are written?
##########
.github/services/hdfs/hdfs_native/action.yml:
##########
@@ -0,0 +1,46 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: hdfs_native
+description: 'Behavior test for hdfs_native'
+
+runs:
+ using: "composite"
+ steps:
+ - name: Setup java env
+ uses: actions/setup-java@v4
+ with:
+ distribution: temurin
+ java-version: "11"
+ - name: Setup
+ shell: bash
+ run: |
+ curl -LsSf https://dlcdn.apache.org/hadoop/common/hadoop-3.3.5/hadoop-3.3.5.tar.gz | tar zxf - -C /home/runner
+
+ export HADOOP_HOME="/home/runner/hadoop-3.3.5"
+ export CLASSPATH=$(${HADOOP_HOME}/bin/hadoop classpath --glob)
+
+ cp ./fixtures/hdfs/hdfs-site.xml ${HADOOP_HOME}/etc/hadoop/hdfs-site.xml
+
+ cat << EOF >> $GITHUB_ENV
+ HADOOP_HOME=${HADOOP_HOME}
+ CLASSPATH=${CLASSPATH}
+ LD_LIBRARY_PATH=${JAVA_HOME}/lib/server:${HADOOP_HOME}/lib/native
+ OPENDAL_HDFS_ROOT=/tmp/opendal/
Review Comment:
They should be `OPENDAL_HDFS_NATIVE_ROOT`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "shbhmrzd (via GitHub)" <gi...@apache.org>.
shbhmrzd commented on PR #4505:
URL: https://github.com/apache/opendal/pull/4505#issuecomment-2068976321
@Xuanwo
Implemented review comments. Please check now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1575585552
##########
.github/services/hdfs_native/hdfs/action.yml:
##########
@@ -0,0 +1,46 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: hdfs_native
+description: 'Behavior test for hdfs_native'
+
+runs:
+ using: "composite"
+ steps:
+ - name: Setup java env
+ uses: actions/setup-java@v4
+ with:
+ distribution: temurin
+ java-version: "11"
+ - name: Setup
+ shell: bash
+ run: |
+ curl -LsSf https://dlcdn.apache.org/hadoop/common/hadoop-3.3.5/hadoop-3.3.5.tar.gz | tar zxf - -C /home/runner
+
+ export HADOOP_HOME="/home/runner/hadoop-3.3.5"
+ export CLASSPATH=$(${HADOOP_HOME}/bin/hadoop classpath --glob)
+
+ cp ./fixtures/hdfs/hdfs-site.xml ${HADOOP_HOME}/etc/hadoop/hdfs-site.xml
+
+ cat << EOF >> $GITHUB_ENV
+ HADOOP_HOME=${HADOOP_HOME}
+ 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
Review Comment:
> 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.
##########
core/src/services/hdfs_native/writer.rs:
##########
@@ -18,25 +18,29 @@
use hdfs_native::file::FileWriter;
use crate::raw::oio;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeWriter {
- _f: FileWriter,
+ f: FileWriter,
}
impl HdfsNativeWriter {
pub fn new(f: FileWriter) -> Self {
- HdfsNativeWriter { _f: f }
+ HdfsNativeWriter { f }
}
}
impl oio::Write for HdfsNativeWriter {
- async fn write(&mut self, _bs: Buffer) -> Result<usize> {
- todo!()
+ async fn write(&mut self, bs: Buffer) -> Result<usize> {
+ let bytes = bs.to_bytes();
+ let total_bytes = bytes.len();
+ self.f.write(bytes).await.map_err(parse_hdfs_error)?;
+ Ok(total_bytes)
Review Comment:
Please don't rely this behavior. Return written like this:
```rust
let n = self.f.write(bytes).await.map_err(parse_hdfs_error)?;
Ok(n)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "shbhmrzd (via GitHub)" <gi...@apache.org>.
shbhmrzd commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1575601357
##########
.github/services/hdfs_native/hdfs/action.yml:
##########
@@ -0,0 +1,46 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+name: hdfs_native
+description: 'Behavior test for hdfs_native'
+
+runs:
+ using: "composite"
+ steps:
+ - name: Setup java env
+ uses: actions/setup-java@v4
+ with:
+ distribution: temurin
+ java-version: "11"
+ - name: Setup
+ shell: bash
+ run: |
+ curl -LsSf https://dlcdn.apache.org/hadoop/common/hadoop-3.3.5/hadoop-3.3.5.tar.gz | tar zxf - -C /home/runner
+
+ export HADOOP_HOME="/home/runner/hadoop-3.3.5"
+ export CLASSPATH=$(${HADOOP_HOME}/bin/hadoop classpath --glob)
+
+ cp ./fixtures/hdfs/hdfs-site.xml ${HADOOP_HOME}/etc/hadoop/hdfs-site.xml
+
+ cat << EOF >> $GITHUB_ENV
+ HADOOP_HOME=${HADOOP_HOME}
+ 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
Review Comment:
Sorry, missed this one. Thank you for pointing this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "shbhmrzd (via GitHub)" <gi...@apache.org>.
shbhmrzd commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1574097994
##########
core/src/services/hdfs_native/reader.rs:
##########
@@ -18,20 +18,40 @@
use hdfs_native::file::FileReader;
use crate::raw::*;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeReader {
- _f: FileReader,
+ f: FileReader,
}
impl HdfsNativeReader {
pub fn new(f: FileReader) -> Self {
- HdfsNativeReader { _f: f }
+ HdfsNativeReader { f }
}
}
impl oio::Read for HdfsNativeReader {
- async fn read_at(&self, _offset: u64, _limit: usize) -> Result<Buffer> {
- 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 {
Review Comment:
Where can we put this check ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #4505:
URL: https://github.com/apache/opendal/pull/4505#issuecomment-2783058217
Thank you @shbhmrzd for your work, the hdfs_native has been supported.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "shbhmrzd (via GitHub)" <gi...@apache.org>.
shbhmrzd commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1574470663
##########
core/src/services/hdfs_native/writer.rs:
##########
@@ -18,25 +18,29 @@
use hdfs_native::file::FileWriter;
use crate::raw::oio;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeWriter {
- _f: FileWriter,
+ f: FileWriter,
}
impl HdfsNativeWriter {
pub fn new(f: FileWriter) -> Self {
- HdfsNativeWriter { _f: f }
+ HdfsNativeWriter { f }
}
}
impl oio::Write for HdfsNativeWriter {
- async fn write(&mut self, _bs: Buffer) -> Result<usize> {
- todo!()
+ async fn write(&mut self, bs: Buffer) -> Result<usize> {
+ let bytes = bs.to_bytes();
+ let total_bytes = bytes.len();
+ self.f.write(bytes).await.map_err(parse_hdfs_error)?;
+ Ok(total_bytes)
Review Comment:
Yes, based on the definition, it continues to write until completer `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)
}
```
##########
core/src/services/hdfs_native/writer.rs:
##########
@@ -18,25 +18,29 @@
use hdfs_native::file::FileWriter;
use crate::raw::oio;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeWriter {
- _f: FileWriter,
+ f: FileWriter,
}
impl HdfsNativeWriter {
pub fn new(f: FileWriter) -> Self {
- HdfsNativeWriter { _f: f }
+ HdfsNativeWriter { f }
}
}
impl oio::Write for HdfsNativeWriter {
- async fn write(&mut self, _bs: Buffer) -> Result<usize> {
- todo!()
+ async fn write(&mut self, bs: Buffer) -> Result<usize> {
+ let bytes = bs.to_bytes();
+ let total_bytes = bytes.len();
+ self.f.write(bytes).await.map_err(parse_hdfs_error)?;
+ Ok(total_bytes)
Review Comment:
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)
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "shbhmrzd (via GitHub)" <gi...@apache.org>.
shbhmrzd commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1574458190
##########
core/src/services/hdfs_native/reader.rs:
##########
@@ -18,20 +18,40 @@
use hdfs_native::file::FileReader;
use crate::raw::*;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeReader {
- _f: FileReader,
+ f: FileReader,
}
impl HdfsNativeReader {
pub fn new(f: FileReader) -> Self {
- HdfsNativeReader { _f: f }
+ HdfsNativeReader { f }
}
}
impl oio::Read for HdfsNativeReader {
- async fn read_at(&self, _offset: u64, _limit: usize) -> Result<Buffer> {
- 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 {
Review Comment:
Sure, created issue #4506
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #4505:
URL: https://github.com/apache/opendal/pull/4505#issuecomment-2556311019
Hi, @shbhmrzd, thank you for your contribution. This PR is quite outdated. Would you prefer to start a new one or resolve the conflicts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo closed pull request #4505: feat(services/hdfs_native): Add read,write,list implementation for hdfs_native
URL: https://github.com/apache/opendal/pull/4505
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "shbhmrzd (via GitHub)" <gi...@apache.org>.
shbhmrzd commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1575567488
##########
.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.
Review Comment:
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 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1574101675
##########
core/src/services/hdfs_native/reader.rs:
##########
@@ -18,20 +18,40 @@
use hdfs_native::file::FileReader;
use crate::raw::*;
+use crate::services::hdfs_native::error::parse_hdfs_error;
use crate::*;
pub struct HdfsNativeReader {
- _f: FileReader,
+ f: FileReader,
}
impl HdfsNativeReader {
pub fn new(f: FileReader) -> Self {
- HdfsNativeReader { _f: f }
+ HdfsNativeReader { f }
}
}
impl oio::Read for HdfsNativeReader {
- async fn read_at(&self, _offset: u64, _limit: usize) -> Result<Buffer> {
- 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 {
Review Comment:
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #4505:
URL: https://github.com/apache/opendal/pull/4505#discussion_r1575549948
##########
core/src/types/scheme.rs:
##########
@@ -454,7 +454,7 @@ impl From<Scheme> for &'static str {
Scheme::Upyun => "upyun",
Scheme::YandexDisk => "yandex_disk",
Scheme::Pcloud => "pcloud",
- Scheme::HdfsNative => "hdfs_native",
+ Scheme::HdfsNative => "hdfs-native",
Review Comment:
Please revert this change
##########
core/src/types/scheme.rs:
##########
@@ -384,7 +384,7 @@ impl FromStr for Scheme {
"tikv" => Ok(Scheme::Tikv),
"azfile" => Ok(Scheme::Azfile),
"mongodb" => Ok(Scheme::Mongodb),
- "hdfs_native" => Ok(Scheme::HdfsNative),
+ "hdfs-native" => Ok(Scheme::HdfsNative),
Review Comment:
Please revert this change
##########
.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.
Review Comment:
Please don't touch those files, hdfs_native is pure rust which should be fine on other bindings.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(services/hdfs_native): Add read,write,list implementation for hdfs_native [opendal]
Posted by "shbhmrzd (via GitHub)" <gi...@apache.org>.
shbhmrzd commented on PR #4505:
URL: https://github.com/apache/opendal/pull/4505#issuecomment-2556447756
> Hi, @shbhmrzd, thank you for your contribution. This PR is quite outdated. Would you prefer to start a new one or resolve the conflicts?
Hi, sorry this got too outdated. I will work on this to wrap it up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org