copy: attempt fallback to full retrieval only when possible#2589
copy: attempt fallback to full retrieval only when possible#2589giuseppe merged 3 commits intocontainers:mainfrom
Conversation
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
|
Cc: @Honny1 |
Honny1
left a comment
There was a problem hiding this comment.
Overall, I like the changes, but I have a few curious questions that don't block this PR.
|
|
||
| // ErrFallbackToOrdinaryLayerDownload is a custom error type returned by PutBlobPartial. | ||
| // It suggests to the caller that a fallback mechanism can be used instead of a hard failure. | ||
| type ErrFallbackToOrdinaryLayerDownload struct { |
There was a problem hiding this comment.
Why is this error defined here when a similar error is created in c/storage? Is it okay to have two same-defined types? Which would be used if they have different fields? I'm just curious.
There was a problem hiding this comment.
we don't want the copy package to have a dependency on containers/storage. In this way, it is possible to build containers/image without the storage backend and avoid the vendoring cost
There was a problem hiding this comment.
(Also, it’s not just the technical dependency; c/image/copy strives to contain “generic” code without hard-coding specifics of transports or image formats.
It doesn’t always succeed — we do have some special cases, and really a lot of the private API is specific to the c/storage design and constraints.)
a4d7889 to
5cebcbe
Compare
|
@mtrmac thanks for the review. Addressed the comments and pushed a new version |
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
LGTM otherwise, please merge after the comment update without another review.
define a new error type so that the caller can determine whether it is safe to ignore the error and retrieve the resource fully. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
5cebcbe to
3dc265d
Compare
thanks! Comments addressed |
Do not attempt the fallback mechanism unless it was reported by the underlying transport.
Follow up for: containers/storage#2118