fix: Allow HTTP object store for json/bson etc. #2784
Conversation
| // TODO: Download the contents of the file and handle using local | ||
| // store (maybe outside of HTTP store). |
There was a problem hiding this comment.
this is what I did in the sqlite from cloud handler, if I understand things correctly.
Also it seems like, we probably don't want to rely on content-length headers being accurate...
The "content-length" part |
vrongmeal
force-pushed
the
vrongmeal/http-obj-store
branch
from
097bc7a to
be09a6e
Compare
| if u.as_str().contains('*') { | ||
| return Err(ObjectStoreSourceError::InvalidHttpStatus(format!( | ||
| "Unexpected status code '{}' for url: '{}'. \ | ||
| Note that globbing is not supported for HTTP.", | ||
| status, u, | ||
| ))); | ||
| } |
There was a problem hiding this comment.
I would have imagined that we'd do this somewhere other than the check for the content length?
| e_tag: None, | ||
| version: None, | ||
| }, | ||
| range: 0..size, |
There was a problem hiding this comment.
is this an array for every byte?
| // TODO: Maybe write the byte stream to file? | ||
| // Would only be useful when the returned bytes are too many. | ||
| let contents = res.bytes().await?; | ||
|
|
||
| let size = contents.len(); | ||
|
|
||
| let stream = async_stream::stream! { | ||
| let res: Result<Bytes, object_store::Error> = Ok(contents); | ||
| yield res; | ||
| }; | ||
|
|
||
| let payload = GetResultPayload::Stream(Box::pin(stream)); |
There was a problem hiding this comment.
my concern here is less that we should write it to a file (or not) but that we actually/unintentionally end up reading the whole stream into memory here... That's the main risk in my mind...
| // TODO: Maybe write the byte stream to file? | ||
| // Would only be useful when the returned bytes are too many. | ||
| let contents = res.bytes().await?; | ||
| let download_file_path = std::env::temp_dir().join(format!("{}", Uuid::new_v4())); |
There was a problem hiding this comment.
what cleans this up? maybe use tmpdir package, which cleans up tempdirs when the tempdir object goes out of scope...
Resolves part of #2756 Signed-off-by: Vaibhav <vrongmeal@gmail.com>
Signed-off-by: Vaibhav <vrongmeal@gmail.com>
Signed-off-by: Vaibhav <vrongmeal@gmail.com>
Signed-off-by: Vaibhav <vrongmeal@gmail.com>
vrongmeal
force-pushed
the
vrongmeal/http-obj-store
branch
from
e5879a2 to
b5edcca
Compare
| download_file.seek(SeekFrom::Start(0)).await?; | ||
|
|
||
| let stream = async_stream::stream! { | ||
| // Delete when the stream is dropped. |
There was a problem hiding this comment.
don't we need to call this with move? I've been tending to use the futures::stream::once() rather than the macro to make this more clear, but if it's in an Arc then I think it ends up working ok?
There was a problem hiding this comment.
The macro expands to async move. Moreover, the compiler would just error if it didn't here.
Fixes: #2756