gps: source cache: adding close() methods #1024
Conversation
| @@ -350,7 +350,7 @@ func (sm *SourceMgr) Release() { | |||
| // until after the doRelease process is done, as doing so could cause the | |||
| // process to terminate before a signal-driven doRelease() call has a chance | |||
| // to finish its cleanup. | |||
| sm.relonce.Do(func() { sm.doRelease() }) | |||
internal/gps/source_manager.go
Outdated
| @@ -148,7 +148,7 @@ var _ SourceManager = &SourceMgr{} | |||
| // gps's SourceManager is intended to be threadsafe (if it's not, please file a | |||
| // bug!). It should be safe to reuse across concurrent solving runs, even on | |||
| // unrelated projects. | |||
| func NewSourceManager(cachedir string) (*SourceMgr, error) { | |||
| func NewSourceManager(cachedir string, log func(v ...interface{})) (*SourceMgr, error) { | |||
There was a problem hiding this comment.
injecting a logger into the SourceManager is something we need to do, but i've been delaying it for...well, not terribly good reasons. it feels kinda awkward to back it in this way, and especially to use a one-off type for doing it.
i'd be more comfortable, at least for now, with architecting out what we need internally, without actually changing the interface for it. sorta analogous to the top-level context.Context object we create in NewSourceManager().
There was a problem hiding this comment.
With regard to the one-off type, I'm happy to use a *log.Logger instead. My WIP branch has more usages, and at some point this seemed cleaner, but maybe it only reduced imports.
Regarding the rest, can you elaborate on what you are suggesting?
For more context, there is currently an additional cacheAge parameter in my WIP branch as well. Both need to be customizable for testing at the very least, so would an unexported constructor be acceptable alternative for now?
Long term, should this API take some sort of forward compatible configuration (e.g. single struct, functional options, etc)?
There was a problem hiding this comment.
The long term vision for the NewSourceManager() API is murky, which is part of why i want to avoid changing it impulsively now. but, yes, i imagine we're gonna head in a struct-like direction.
an unexported constructor that sets a logger for test purposes would be 👍 for now.
There was a problem hiding this comment.
Looking at this again, the log func type actually made plugging in test/bench log functions much cleaner, but they can be adapted to log.Logger if needed.
More importantly, I didn't think through the unexported constructor all the way. It falls short of our needs (or it will in a near future PR) since it can't be accessed by the commands.
Can we start heading in a struct-like direction now? The whole package is still internal, so nothing public is thrashing.
There was a problem hiding this comment.
Pushed an update moving to a ConfigStruct{}.New() sort of api:
sm, err := gps.SourceManagerConfig{
Cachedir: cachedir,
Logger: logger,
}.NewSourceManager()Happy to continue to play with the API.
Opinions on shortening the method to New()?
There was a problem hiding this comment.
👍 to shortening to New().
is this config-method-to-create a common thing? don't think i've run across it before.
There was a problem hiding this comment.
I'm not sure. I hadn't really thought about it as a 'pattern', but it certainly seems to serve this case well and I'll keep it in mind in the future.
There was a problem hiding this comment.
i'm on the fence. it's basically making a factory out of the config type, which is...well, yeah, i'm on the fence.
can we go back to just NewSourceManager(SourceManagerConfig{}) for now, and we can discuss this particular pattern in a standalone PR?
internal/gps/source_manager.go
Outdated
| @@ -148,8 +153,12 @@ var _ SourceManager = &SourceMgr{} | |||
| // gps's SourceManager is intended to be threadsafe (if it's not, please file a | |||
| // bug!). It should be safe to reuse across concurrent solving runs, even on | |||
| // unrelated projects. | |||
| func NewSourceManager(cachedir string) (*SourceMgr, error) { | |||
| err := os.MkdirAll(filepath.Join(cachedir, "sources"), 0777) | |||
| func (c SourceManagerConfig) NewSourceManager() (*SourceMgr, error) { | |||
There was a problem hiding this comment.
Why not change NewSourceManager(cachedir string) into NewSourceManager(c SourceManagerConfig)?
There was a problem hiding this comment.
Or even just NewSourceManager(cachedir string, logger *log.Logger)?
There was a problem hiding this comment.
Why not change NewSourceManager(cachedir string) into NewSourceManager(c SourceManagerConfig)?
I thought it was more verbose than it needed to be, especially for callers outside of gps:
gps.NewSourceManager(gps.SourceManagerConfig{...})Or even just NewSourceManager(cachedir string, logger *log.Logger)?
There will be more parameters added in the future, and some will be optional (logger already is), so we wanted to avoid an ever expanding exported constructor.
| @@ -59,6 +59,9 @@ type singleSourceCache interface { | |||
| // If the input is a revision and multiple UnpairedVersions are associated | |||
| // with it, whatever happens to be the first is returned. | |||
| toUnpaired(v Version) (UnpairedVersion, bool) | |||
|
|
|||
| // close closes the cache and any backing resources. | |||
| close() error | |||
There was a problem hiding this comment.
Embed an io.Closer in the interface instead?
There was a problem hiding this comment.
Given that the doc is so open ended for this particular interface, I'm not sure that it buys us much. I also hesitate to export a method before it needs to be. I don't really have strong feelings either way though.
There was a problem hiding this comment.
Neither do I. Just a suggestion. 😁
There was a problem hiding this comment.
yeah let's please keep this unexported and not worry aboutio.Closer()
…Gateway, and sourceCoordinator
|
just the rebase, then i think this is 👍 |
What does this do / why do we need it?
Adds
close()methods tosingleSourceCache,sourceGateway, andsourceCoordinator. Closing an in-memory cache is a no-op, so nothing much is changing here, but persistent caches will have backing files and this reduces the size of that future PR.What should your reviewer look out for in this PR?
Nits for doc language, variable names, api.
Which issue(s) does this PR fix?
Related to #431