You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "cuichenli (via GitHub)" <gi...@apache.org> on 2023/05/11 06:01:14 UTC
[GitHub] [incubator-opendal] cuichenli opened a new pull request, #2253: feat(core): add presign support for obs
cuichenli opened a new pull request, #2253:
URL: https://github.com/apache/incubator-opendal/pull/2253
fixes: #2070
noticed that oss already have the presign support, so this pr only touches obs.
--
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] suyanhanx merged pull request #2253: feat(core): add presign support for obs
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx merged PR #2253:
URL: https://github.com/apache/incubator-opendal/pull/2253
--
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] suyanhanx commented on a diff in pull request #2253: feat(core): add presign support for obs
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #2253:
URL: https://github.com/apache/incubator-opendal/pull/2253#discussion_r1190735610
##########
core/src/services/obs/core.rs:
##########
@@ -254,4 +317,16 @@ impl ObsCore {
self.send(req).await
}
+
+ pub async fn sign_query<T>(&self, req: &mut Request<T>, duration: Duration) -> Result<()> {
Review Comment:
Can you please put this method together with `sign`? Thank you.
https://github.com/apache/incubator-opendal/blob/5552c9741a754067d54d1f2c065fc79bc6871add/core/src/services/obs/core.rs#L70
--
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 pull request #2253: feat(core): add presign support for obs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2253:
URL: https://github.com/apache/incubator-opendal/pull/2253#issuecomment-1543416890
> not sure if it would be worth to extract the shared logic?
We can call `xxx_request` in `xxx`.
> also for the `sign_query` method, not sure if it would be worth to make it as a trait?
Make it a trait is not helpful because one service one has one valid `sign_query` implementaion.
--
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] cuichenli commented on pull request #2253: feat(core): add presign support for obs
Posted by "cuichenli (via GitHub)" <gi...@apache.org>.
cuichenli commented on PR #2253:
URL: https://github.com/apache/incubator-opendal/pull/2253#issuecomment-1543688300
thanks, updated based on the comments above.
--
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] cuichenli commented on pull request #2253: feat(core): add presign support for obs
Posted by "cuichenli (via GitHub)" <gi...@apache.org>.
cuichenli commented on PR #2253:
URL: https://github.com/apache/incubator-opendal/pull/2253#issuecomment-1543386897
some side notes:
noticed there are a lot of code duplicates, for instance: `obs_get_head_object_request` and `obs_get_head_object` share the same logic on setup the `req` object, not sure if it would be worth to extract the shared logic?
also for the `sign_query` method, not sure if it would be worth to make it as a trait?
--
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 #2253: feat(core): add presign support for obs
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2253:
URL: https://github.com/apache/incubator-opendal/pull/2253#discussion_r1190695313
##########
core/src/services/obs/core.rs:
##########
@@ -118,6 +119,38 @@ impl ObsCore {
self.send(req).await
}
+ pub fn obs_get_object_request(
Review Comment:
`obs_get_object` can call `obs_get_object_request` instead.
##########
core/src/services/obs/core.rs:
##########
@@ -180,6 +213,36 @@ impl ObsCore {
self.send(req).await
}
+ pub fn obs_get_head_object_request(
Review Comment:
Oh, `obs_get_head_object` is wrong, please change into `obs_head_object`.
And `obs_head_object` can call `obs_head_object_request`
--
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