-
Notifications
You must be signed in to change notification settings - Fork 887
Conversation
63dd534
to
4e696a5
Compare
1e1a255
to
2a2161d
Compare
Needs rebase/update. |
@iaguis @jonboulle Updated. Just a question, as rocket is in development, does this patch needs a migrate script (to create the new tables on an already populated db? (if someone used rkt between #361 and this)). Plus the acis already in the db won't have an entry in the table (and this can bring some problems when GetACI will be used). I usually clean my local cas data when I see strange problems/errors (in past this happened some times). For the final user, should it be warned, at release time, of the need to clean its cas? There are other various cases:
|
@sgotti I am not concerned about migration yet as we are still getting the DB in place; obviously it will become a requirement at a later point once we have a stable on-disk format, but for now I am happy to just advise people to clear their cas. |
) | ||
|
||
// ACIInfo is used to store informations about an ACI | ||
// BlobKey is the key in the blob and imageManifest store of the related ACI file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to just inline these docstrings, for example - http://golang.org/pkg/net/http/#Response
@jonboulle thanks. I hope to have fixed all your comments. In the meantime I expanded the tests and added an index on appname as I think it will be one of the most used search criteria. |
// was imported in the store. Latest defines if the ACI was imported using the | ||
// latest pattern (no version label was provided on ACI discovery) | ||
type ACIInfo struct { | ||
BlobKey string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, could you please update this? what I meant was
// ACIInfo is used to store information about an ACI
type ACIInfo struct {
// BlobKey is the key in the ...
BlobKey string
// AppName is the ...
AppName string
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! Sorry... Now I overlooked you explanation.
LGTM |
The ACIInfo table save informations on the ACIs. The saved informations are: * BlobKey is the key in the blob and imageManifest store of the related ACI file. * AppName is the app name provided by the ACI. * ImportTime is the time this ACI was imported in the store * Latest defines if the ACI was imported using the latest pattern (no version label provided on ACI discovery)
When testing with this patch merged, I had this error:
Deleting |
Yes that is a known issue, we should make sure we note it in the release
|
Last commit is the interesting one. It needs the db implementation provided in #361 and is based over #392. IMHO this db based implementation is really cleaner and better that the hackish diskv based ones proposed in #352 and #353.
The ACIInfo table saves informations on the ACIs. The saved informations are:
label provided on ACI discovery)
About the latest pattern please see (and comment) on appc/spec#73.