Add HttpStore (#3294) #3380
Add HttpStore (#3294) #3380
Conversation
tustvold
force-pushed
the
add-http-store
branch
from
4dca8a0
to
b69c595
Compare
tustvold
force-pushed
the
add-http-store
branch
from
b69c595
to
7978b2b
Compare
|
@roeap not sure if this is something you might be interested in reviewing, absolutely no pressure though 😅 |
There was a problem hiding this comment.
LGTM! Had only few cosmetic comments.
One question that is likely somehow taken care of is deleting directories. SInce the http store has a concept of directories, what happens (or should happen) to these when we delete an object that has parent directories that are empty. From what I can tell these would just keep on existing. Since we exclude directories when listing files, no harm done, but what happens to the common_prefixes ? Was not 100% sue from the code :)
| match self.prop_stat.status.contains("200 OK") { | ||
| true => Ok(()), | ||
| false => Err(Error::PropStatus { | ||
| href: self.href.clone(), | ||
| status: self.prop_stat.status.clone(), | ||
| } | ||
| .into()), |
There was a problem hiding this comment.
Would that be simpler as if / else?
There was a problem hiding this comment.
It ends up being more lines of code, so I'm inclined to leave it
| match response.is_dir() { | ||
| true => Err(Error::IsDirectory.into()), | ||
| false => response.object_meta(self.client.base_url()), | ||
| } |
There was a problem hiding this comment.
same as before, albeit here I fine the match syntax quite concise 😆
| retry_config: RetryConfig, | ||
| } | ||
|
|
||
| impl HttpBuilder { |
There was a problem hiding this comment.
should we add a from_env method here as well, to pick up the HTTP_URL variable? That said, while i don't see much conflicting configs in the env, HTTP_URL is about as generic as it gets 😆 - is something like HTTP_STORE_URL maybe more telling?
There was a problem hiding this comment.
I think unlike S3 or GCP there aren't standard environment variables to use for this purpose, mirroring those used by an SDK, so I think best to leave it for now
Yeah, it is not an ideal situation, but is consistent with how we handle this, or more specifically don't handle this, for |
|
Benchmark runs are scheduled for baseline = e7fc073 and contender = 98bba9c. 98bba9c is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Which issue does this PR close?
Closes #3294
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?