You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "parkma99 (via GitHub)" <gi...@apache.org> on 2023/07/13 12:07:46 UTC
[GitHub] [incubator-opendal] parkma99 opened a new pull request, #2634: add detect_region code for S3Builder
parkma99 opened a new pull request, #2634:
URL: https://github.com/apache/incubator-opendal/pull/2634
close #2632
I am newer for `OpenDAL` and this is my first PR for `OpenDAL`.
Now the test case for `detect_region` is not added.
I am confusing how to build a `S3Builder` with custom `region`, default is None, and use `region(str)` function needs the `S3Builder` has a custom `region`.
BTW, how check `Error` in test case, I try use `assert_eq!()` for check `Error`, it failed.
Thanks for helping.
--
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 #2634: add detect_region code for S3Builder
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #2634:
URL: https://github.com/apache/incubator-opendal/pull/2634#discussion_r1263370423
##########
core/src/services/s3/backend.rs:
##########
@@ -517,6 +518,60 @@ impl S3Builder {
self
}
+
+ /// a helper function to make it easier to find region
Review Comment:
This is a public function to user, we should:
- Add detailed explaination on the input value.
- Add detailed explaination on the return value, like `Some(v)`, `None`
- Add an example for this API.
##########
core/src/services/s3/backend.rs:
##########
@@ -517,6 +518,60 @@ impl S3Builder {
self
}
+
+ /// a helper function to make it easier to find region
+ pub async fn detect_region(&self) -> Result<Option<String>> {
+ if let Some(region) = &self.region {
+ return Ok(Some(region.to_string()));
+ }
+
+ let mut endpoint = match &self.endpoint {
+ Some(endpoint) => {
+ if endpoint.starts_with("http") {
+ endpoint.to_string()
+ } else {
+ // Prefix https if endpoint doesn't start with scheme.
+ format!("https://{}", endpoint)
+ }
+ }
+ None => "https://s3.amazonaws.com".to_string(),
+ };
+
+ // If endpoint contains bucket name, we should trim them.
+ endpoint = endpoint.replace(&format!("//{0}.", &self.bucket), "//");
+
+ let url = format!("{0}/{1}", endpoint, &self.bucket);
+
+ debug!("backend detect region with url: {url}");
+
+ let req = http::Request::head(&url).body(AsyncBody::Empty).map_err(new_request_build_error)?;
+
+ let client = HttpClient::new()?;
+ let res = client.send(req).await?;
+
+ debug!(
+ "auto detect region got response: status {:?}, header: {:?}",
+ res.status(),
+ res.headers()
+ );
+
+ match res.status() {
+ // The endpoint works, return with not changed endpoint and
+ // default region.
+ StatusCode::OK | StatusCode::FORBIDDEN | StatusCode::METHOD_NOT_ALLOWED => {
+ let region = res
+ .headers()
+ .get("x-amz-bucket-region")
+ .unwrap_or(&HeaderValue::from_static("us-east-1"))
Review Comment:
We should not use `us-east-1` as default, return `Ok(None)` instead.
##########
core/src/services/s3/backend.rs:
##########
@@ -517,6 +518,60 @@ impl S3Builder {
self
}
+
+ /// a helper function to make it easier to find region
+ pub async fn detect_region(&self) -> Result<Option<String>> {
Review Comment:
I feel like we should not read any value from builder which is confusing and hard to use. We can have an API like:
```
pub async fn detect_region(endpoint: &str, bucket: &str) -> Option<String> {
```
##########
core/src/services/s3/backend.rs:
##########
@@ -517,6 +518,60 @@ impl S3Builder {
self
}
+
+ /// a helper function to make it easier to find region
+ pub async fn detect_region(&self) -> Result<Option<String>> {
+ if let Some(region) = &self.region {
+ return Ok(Some(region.to_string()));
+ }
+
+ let mut endpoint = match &self.endpoint {
+ Some(endpoint) => {
+ if endpoint.starts_with("http") {
+ endpoint.to_string()
+ } else {
+ // Prefix https if endpoint doesn't start with scheme.
+ format!("https://{}", endpoint)
+ }
+ }
+ None => "https://s3.amazonaws.com".to_string(),
+ };
+
+ // If endpoint contains bucket name, we should trim them.
+ endpoint = endpoint.replace(&format!("//{0}.", &self.bucket), "//");
+
+ let url = format!("{0}/{1}", endpoint, &self.bucket);
+
+ debug!("backend detect region with url: {url}");
+
+ let req = http::Request::head(&url).body(AsyncBody::Empty).map_err(new_request_build_error)?;
+
+ let client = HttpClient::new()?;
+ let res = client.send(req).await?;
+
+ debug!(
+ "auto detect region got response: status {:?}, header: {:?}",
+ res.status(),
+ res.headers()
+ );
+
+ match res.status() {
+ // The endpoint works, return with not changed endpoint and
+ // default region.
+ StatusCode::OK | StatusCode::FORBIDDEN | StatusCode::METHOD_NOT_ALLOWED => {
+ let region = res
+ .headers()
+ .get("x-amz-bucket-region")
+ .unwrap_or(&HeaderValue::from_static("us-east-1"))
+ .to_str()
+ .map_err(|e| println!("{e:?}")).unwrap()
+ .to_string();
+ Ok(Some(region))
+ }
+ // Unexpected status code
+ code => Err(Error::new(ErrorKind::NotFound, format!("can't detect region automatically, no valid endpoint template for {endpoint} {code:?}").as_str()))
Review Comment:
If we failed to find the region (no matter why), we should better return `None` instead.
##########
core/src/services/s3/backend.rs:
##########
@@ -517,6 +518,60 @@ impl S3Builder {
self
}
+
+ /// a helper function to make it easier to find region
+ pub async fn detect_region(&self) -> Result<Option<String>> {
+ if let Some(region) = &self.region {
+ return Ok(Some(region.to_string()));
+ }
+
+ let mut endpoint = match &self.endpoint {
+ Some(endpoint) => {
+ if endpoint.starts_with("http") {
+ endpoint.to_string()
+ } else {
+ // Prefix https if endpoint doesn't start with scheme.
+ format!("https://{}", endpoint)
+ }
+ }
+ None => "https://s3.amazonaws.com".to_string(),
+ };
+
+ // If endpoint contains bucket name, we should trim them.
+ endpoint = endpoint.replace(&format!("//{0}.", &self.bucket), "//");
+
+ let url = format!("{0}/{1}", endpoint, &self.bucket);
+
+ debug!("backend detect region with url: {url}");
+
+ let req = http::Request::head(&url).body(AsyncBody::Empty).map_err(new_request_build_error)?;
+
+ let client = HttpClient::new()?;
+ let res = client.send(req).await?;
+
+ debug!(
+ "auto detect region got response: status {:?}, header: {:?}",
+ res.status(),
+ res.headers()
+ );
+
+ match res.status() {
+ // The endpoint works, return with not changed endpoint and
+ // default region.
+ StatusCode::OK | StatusCode::FORBIDDEN | StatusCode::METHOD_NOT_ALLOWED => {
+ let region = res
+ .headers()
+ .get("x-amz-bucket-region")
+ .unwrap_or(&HeaderValue::from_static("us-east-1"))
+ .to_str()
+ .map_err(|e| println!("{e:?}")).unwrap()
Review Comment:
Please return error to user instead of print it and unwrap.
--
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 merged pull request #2634: feat(services/s3): Add detect_region support for S3Builder
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #2634:
URL: https://github.com/apache/incubator-opendal/pull/2634
--
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 #2634: add detect_region code for S3Builder
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #2634:
URL: https://github.com/apache/incubator-opendal/pull/2634#issuecomment-1634150493
Thanks for the PR! Let me make the feature request more clear.
As described in the issue, I want an API of `S3Builder` like `async fn detect_region(&self) -> Result<Option<String>>`, it's used to fetch `region` with input `endpoint` and `bucket`. (Maybe API should be `async fn detect_region(&self, endpoint: &str, bucket: &str)`?)
The API should use the client in `S3Builder` to detect the region, the behavior will be similiar to
https://github.com/apache/incubator-opendal/blob/44e93563227e58520f07e0ebcf3be14601b59d9a/src/services/s3/backend.rs#L504-L604
The caller can use this function to detect region when they needed in async context. (Please note that `S3Builder::build` is blocking API, so we can't do 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