-
Notifications
You must be signed in to change notification settings - Fork 247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move locking responsibility from store.go to the container/image/layer stores #1395
Conversation
... so that we can modify/replace them. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This integrates ReloadIfChanges, and makes it clearer that the responsibility for maintaining locking details is with the containerStore; we can change it in a single place. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This integrates ReloadIfChanges, and makes it clearer that the responsibility for maintaining locking details is with the containerStore; we can change it in a single place. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Exposing the internals of the lock is not necessary, and exposes too many implementation details. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
The only callers are internal, have them access r.lockfile directly. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Callers should just use startReading/startWriting. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
It is done implicitly by all writers already. Also fix the documentation not to point at an explicit Touch(), which is not actually necessary. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
They are now only used in the constructor, so use a variant of startWriting instead. This code path is not performance-critical, so let's share as much code as possible to ensure consistency in locking. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... to be closer to the lock / load set of methods. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... so that we can modify/replace them. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This integrates ReloadIfChanged, and makes it clearer that the responsibility for maintaining locking details is with the imageStore; we can change it in a single place. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This integrates ReloadIfChanged, and makes it clearer that the responsibility for maintaining locking details is with the imageStore; we can change it in a single place. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... for a bit of extra safety. That requires us to be a bit more explicit in one of the users. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Exposing the internals of the lock is not necessary, and exposes too many implementation details. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
The only callers are internal, have them access r.lockfile directly. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Callers should just use startReading/startWriting. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
rwImageStore.Create() already calls it. Signed-off-by: Miloslav Trmač <[email protected]>
It is done implicitly by all writers already. Also fix the documentation not to point at an explicit Touch(), which is not actually necessary. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
They are now only used in the constructors, so use a variant of startReading/startWriting instead. This code path is not performance-critical, so let's share as much code as possible to ensure consistency in locking. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... to be closer to the lock / load set of methods. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... so that we can modify/replace them. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This integrates ReloadIfChanged, and makes it clearer that the responsibility for maintaining locking details is with the layerStore; we can change it in a single place. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
This integrates ReloadIfChanged, and makes it clearer that the responsibility for maintaining locking details is with the layerStore; we can change it in a single place. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
... for a bit of extra safety. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Exposing the internals of the lock is not necessary, and exposes too many implementation details. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
The only callers are internal, have them access r.lockfile directly. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
It is an internal helper for ReloadIfChanged, with no external users. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Callers should just use startReading/startWriting. Signed-off-by: Miloslav Trmač <[email protected]>
It is done implicitly by all writers already. Also fix the documentation not to point at an explicit Touch(), which is not actually necessary. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
They are now only used in the constructors, so use a variant of startReading/startWriting instead. This code path is not performance-critical, so let's share as much code as possible to ensure consistency in locking. Should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
.. to be closer to the lock / load set of methods. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač <[email protected]>
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.
LGTM
@nalind @flouthoc @giuseppe @vrothberg @saschagrunert PTAL
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.
LGTM
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.
LGTM
Final big part of the preparatory infrastructure work on locking.
Move the conceptual responsibility for locking/loading
containerStore
,imageStore
andlayerStore
from the caller (store
) into the three individual C/I/L objects. And in those objects, centralize it.I.e. this goes from
to
that will, finally, allow us to change the design in a single place,
startReading
/startWriting
. E.g. to:Locked()
orIsReadWrite
in theLoad
method, passing the values from the (now single) caller instead (lockfile.Locked() must not be used to make any decisions #1388 )This should not change observable behavior, apart from the “Remove a redundant rwImageStore.Save() call” commit.
See individual commit messages for details.
(The need to change every lock caller in
store
by this PR is the primary motivation for preparing #1383 / #1384 , which decreased the number of places to update.)This is organized by the individual store, changing each one separately; i.e. each conceptual change is repeated three times. If desired, I can instead reorganize this to do same conceptual change in a single commit, across all three stores.
This ends up removing
roFileBasedStore
/rwFileBasedStore
, and declaring the{start,stop}{Reading,Writing}
methods in each individual store. If preferred, I could change this PR to keep (or re-introduce) those interfaces again.