layers: support BigData #787
Conversation
|
LGTM |
24a89bc to
3a3b51b
Compare
3a3b51b to
11ee17b
Compare
There was a problem hiding this comment.
A literally one-minute impression, and design concerns like this are definitely decided by @nalind , not me:
This can make sense as an internal implementation mechanism; it’s not obvious to me that this must be exposed as a raw data to external callers. If c/image is only going to consume that data through a zstd:chunked - specific API, only that zstd:chunked - specific API needs to be public.
Making raw data public like this
- effectively freezes the format of the raw data, slowing down future improvements.
- invites other uses of raw data for unexpected purposes. That can be very useful and allow for unexpected innovations, OTOH it also further cements down the concept of individual layers and makes it even harder to significantly restructure (e.g. to a file-based approach like OSTree or the chunked format).
Again, definitely up to @nalind, just airing my first impression.
layers.go
Outdated
|
|
||
| // BigDataSizes maps the names in BigDataNames to the sizes of the data | ||
| // that has been stored, if they're known. | ||
| BigDataSizes map[string]int64 `json:"big-data-sizes,omitempty"` |
There was a problem hiding this comment.
Just reading the API documentation, I would expect BigDataSizes to be included in ImageSize. (Otherwise it’s not even clear to me there needs to be a separate faster-to-access record of the sizes.)
There was a problem hiding this comment.
I've only added it because it was present in the image API. I am fine with dropping it
There was a problem hiding this comment.
Why shouldn’t the size be included in ImageSize?
There was a problem hiding this comment.
Let's drop it.
dropped and pushed a new version.
There was a problem hiding this comment.
Why shouldn’t the size be included in
ImageSize?
it is not used at the moment, we can add it if there is any user of it
There was a problem hiding this comment.
The Podman repo suggests that store.ImageSize is called for “disk usage statistics” (df?), “inspect”, “prune” and “tree”; I didn’t trace the full call stack, it may be exposed via other features or it might be dead code.
OTOH it’s true that c/image’s types.Image.Size() implementation does its own calculation which does not currently benefit from extending store.ImageSize.
44b1af2 to
3e4cb85
Compare
|
is there anything more left to address? |
|
See the 2 points from my previous review. |
just replied to them |
|
Not from me. Just waiting for @mtrmac comment. |
|
*shrug* I don’t have anything to add WRT the implementation; I have some concerns but they are not quite blocking. As for the more general design review, I know very little about c/storage , that would be up to @nalind I think. |
|
how to move forward? This is just a subset of the changes I'd like to get merged for zstd:chunked support |
|
@vrothberg @nalind PTAL so we can get this merged. |
|
Added to my todo list. I won't make it today but promise to have a look on Monday. Apologies for not having looked into it until now! |
3e4cb85 to
a975d8d
Compare
a975d8d to
755130c
Compare
similarly to containers and images, add support for storing big data also for layers, so that it is possible to store arbitrary data when it is not feasible to embed it in the layers JSON metadata file. Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
755130c to
55bb805
Compare
|
LGTM |
similarly to containers and images, add support for storing big data
also for layers, so that it is possible to store arbitrary data when
it is not feasible to embed it in the layers JSON metadata file.
Subset of: #775
Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com