You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Zheaoli (via GitHub)" <gi...@apache.org> on 2023/09/30 15:33:53 UTC
[GitHub] [incubator-opendal] Zheaoli opened a new pull request, #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Zheaoli opened a new pull request, #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221
Signed-off-by: Manjusaka <me...@manjusaka.me>
--
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/persy): change blocking_x in async_x call to tokio::task::blocking_spawn [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221
--
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
[GitHub] [incubator-opendal] Zheaoli commented on a diff in pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#discussion_r1341976478
##########
core/src/types/error.rs:
##########
@@ -426,6 +428,16 @@ impl From<Error> for io::Error {
}
}
+impl From<task::JoinError> for Error {
Review Comment:
Actually, I do not get the point.
You mean the people will the Error in OpenDAL to a place different but related to tokio::tasks:JoinError?
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#discussion_r1341975979
##########
core/src/types/error.rs:
##########
@@ -426,6 +428,16 @@ impl From<Error> for io::Error {
}
}
+impl From<task::JoinError> for Error {
Review Comment:
We should not impl this. Users could convert tokio error to Error wrongly. Instead, we should have a new_tokio_error util function.
--
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
[GitHub] [incubator-opendal] oowl commented on a diff in pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "oowl (via GitHub)" <gi...@apache.org>.
oowl commented on code in PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#discussion_r1341979320
##########
core/src/services/persy/backend.rs:
##########
@@ -175,7 +176,12 @@ impl kv::Adapter for Adapter {
}
async fn get(&self, path: &str) -> Result<Option<Vec<u8>>> {
- self.blocking_get(path)
+ let cloned_self = self.clone();
+ let cloned_path = path.to_string();
+ task::spawn_blocking(move || cloned_self.blocking_get(cloned_path.as_str()))
Review Comment:
From my perspective, This may make us have to rely on tokio, and it may be difficult to support other asynchronous runtimes.
--
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
[GitHub] [incubator-opendal] oowl commented on a diff in pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "oowl (via GitHub)" <gi...@apache.org>.
oowl commented on code in PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#discussion_r1341979958
##########
core/src/services/persy/backend.rs:
##########
@@ -175,7 +176,12 @@ impl kv::Adapter for Adapter {
}
async fn get(&self, path: &str) -> Result<Option<Vec<u8>>> {
- self.blocking_get(path)
+ let cloned_self = self.clone();
+ let cloned_path = path.to_string();
+ task::spawn_blocking(move || cloned_self.blocking_get(cloned_path.as_str()))
Review Comment:
cc @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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#discussion_r1341976975
##########
core/src/types/error.rs:
##########
@@ -426,6 +428,16 @@ impl From<Error> for io::Error {
}
}
+impl From<task::JoinError> for Error {
Review Comment:
Implement From<xxx> for Error could make our developers create opendal error unexpectedly via `?` which may lack of enough context.
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#discussion_r1341988275
##########
core/src/services/persy/backend.rs:
##########
@@ -175,7 +176,12 @@ impl kv::Adapter for Adapter {
}
async fn get(&self, path: &str) -> Result<Option<Vec<u8>>> {
- self.blocking_get(path)
+ let cloned_self = self.clone();
+ let cloned_path = path.to_string();
+ task::spawn_blocking(move || cloned_self.blocking_get(cloned_path.as_str()))
Review Comment:
> I think it's ok to add tokio::task here and decide how to fix it when we are trying to support other runtimes.
I'd rather move on than wait for a *perfect* solution, unless the drawbacks are unaffordable.
--
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
[GitHub] [incubator-opendal] Zheaoli commented on pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#issuecomment-1742044458
I will split the error refactor into a single pr. This PR would be drafted first.
--
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
[GitHub] [incubator-opendal] Zheaoli commented on a diff in pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#discussion_r1341979767
##########
core/src/services/persy/backend.rs:
##########
@@ -175,7 +176,12 @@ impl kv::Adapter for Adapter {
}
async fn get(&self, path: &str) -> Result<Option<Vec<u8>>> {
- self.blocking_get(path)
+ let cloned_self = self.clone();
+ let cloned_path = path.to_string();
+ task::spawn_blocking(move || cloned_self.blocking_get(cloned_path.as_str()))
Review Comment:
Good catch, I think we need to make a choice between portable and performance. Or we need to abstract a layer to adapt the different async runtimes.
off-topic, would you mind introducing to me another popular rust async runtime except for tokio?
--
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
[GitHub] [incubator-opendal] Xuanwo commented on a diff in pull request #3221: feat(services/persy): change blocking_x in async_x call to tokio::task::blocking_spawn
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3221:
URL: https://github.com/apache/incubator-opendal/pull/3221#discussion_r1341985876
##########
core/src/services/persy/backend.rs:
##########
@@ -175,7 +176,12 @@ impl kv::Adapter for Adapter {
}
async fn get(&self, path: &str) -> Result<Option<Vec<u8>>> {
- self.blocking_get(path)
+ let cloned_self = self.clone();
+ let cloned_path = path.to_string();
+ task::spawn_blocking(move || cloned_self.blocking_get(cloned_path.as_str()))
Review Comment:
OpenDAL is tokio only for now. I think it's ok to add tokio::task here and decide how to fix it when we are trying to support other runtimes.
--
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