You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "tustvold (via GitHub)" <gi...@apache.org> on 2023/05/12 16:59:45 UTC
[GitHub] [arrow-rs] tustvold opened a new pull request, #4212: Add ObjectStore::get_opts (#2241)
tustvold opened a new pull request, #4212:
URL: https://github.com/apache/arrow-rs/pull/4212
_Draft as needs some cleanup_
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #2241
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #4212: Add ObjectStore::get_opts (#2241)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #4212:
URL: https://github.com/apache/arrow-rs/pull/4212#issuecomment-1547607490
> what do you think about marking ObjectStore::get and ObjectStore::get_range as depreated
I think the fact we provide default implementations is probably sufficient, I would rather not force all users to use `GetOptions` as the API is a little more clunky
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on pull request #4212: Add ObjectStore::get_opts (#2241)
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #4212:
URL: https://github.com/apache/arrow-rs/pull/4212#issuecomment-1547657366
> I think the fact we provide default implementations is probably sufficient, I would rather not force all users to use GetOptions as the API is a little more clunky
I think `get_range` might still be worth deprecating -- I agree that `get` is a nice convenience that we could/should keep
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] alamb commented on a diff in pull request #4212: Add ObjectStore::get_opts (#2241)
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #4212:
URL: https://github.com/apache/arrow-rs/pull/4212#discussion_r1193128952
##########
object_store/src/lib.rs:
##########
@@ -558,6 +575,62 @@ pub struct ObjectMeta {
pub e_tag: Option<String>,
}
+/// Options for a get request
+#[derive(Debug, Default)]
+pub struct GetOptions {
+ /// Request will succeed if the ETag matches
Review Comment:
Could you please define `ETag` somewhere? I am not familiar with the term and I think a summary would be nicer UX than a IETF RFC link)
Also could we summarize explicitly what happens if the option is set bit the request doesn't match (e.g "request will _only_ succeed" (if that is indeed the case))
##########
object_store/src/aws/mod.rs:
##########
@@ -246,39 +247,26 @@ impl ObjectStore for AmazonS3 {
.await
}
- async fn get(&self, location: &Path) -> Result<GetResult> {
- let response = self.client.get_request(location, None, false).await?;
+ async fn get_opts(&self, location: &Path, options: GetOptions) -> Result<GetResult> {
+ let response = self.client.get_request(location, options, false).await?;
let stream = response
.bytes_stream()
.map_err(|source| crate::Error::Generic {
- store: "S3",
+ store: STORE,
source: Box::new(source),
})
.boxed();
Ok(GetResult::Stream(stream))
}
- async fn get_range(&self, location: &Path, range: Range<usize>) -> Result<Bytes> {
Review Comment:
Where did this go?
##########
object_store/src/lib.rs:
##########
@@ -346,11 +346,24 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static {
}
/// Return the bytes that are stored at the specified location.
- async fn get(&self, location: &Path) -> Result<GetResult>;
+ async fn get(&self, location: &Path) -> Result<GetResult> {
+ self.get_opts(location, GetOptions::default()).await
+ }
+
+ /// Perform a get request with options
+ ///
+ /// Note: options.range will be ignored if [`GetResult::File`]
+ async fn get_opts(&self, location: &Path, options: GetOptions) -> Result<GetResult>;
Review Comment:
I think this is a breaking API change and thus we will need to make it a major version number upgrade -- any existing `impl ObjectStore` will stop compiling. I see you have marked the API as such 👍
##########
object_store/src/lib.rs:
##########
@@ -558,6 +575,62 @@ pub struct ObjectMeta {
pub e_tag: Option<String>,
}
+/// Options for a get request
Review Comment:
I think it would help if we mentioned "for example, request range"
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #4212: Add ObjectStore::get_opts (#2241)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold merged PR #4212:
URL: https://github.com/apache/arrow-rs/pull/4212
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #4212: Add ObjectStore::get_opts (#2241)
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on code in PR #4212:
URL: https://github.com/apache/arrow-rs/pull/4212#discussion_r1193649074
##########
object_store/src/aws/mod.rs:
##########
@@ -246,39 +247,26 @@ impl ObjectStore for AmazonS3 {
.await
}
- async fn get(&self, location: &Path) -> Result<GetResult> {
- let response = self.client.get_request(location, None, false).await?;
+ async fn get_opts(&self, location: &Path, options: GetOptions) -> Result<GetResult> {
+ let response = self.client.get_request(location, options, false).await?;
let stream = response
.bytes_stream()
.map_err(|source| crate::Error::Generic {
- store: "S3",
+ store: STORE,
source: Box::new(source),
})
.boxed();
Ok(GetResult::Stream(stream))
}
- async fn get_range(&self, location: &Path, range: Range<usize>) -> Result<Bytes> {
Review Comment:
The default implementation is sufficient
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org