[datastore] Generic 'Dial' function #1409
Conversation
| scdV1Server, err = createSCDServer(ctx, logger) | ||
| if err != nil { | ||
| ridV1Server.Cron.Stop() | ||
| ridV2Server.Cron.Stop() |
There was a problem hiding this comment.
This will be fixed in a follow up PR. It's needed, but not implemented correctly (e.g. missing in scd)
| @@ -0,0 +1,64 @@ | |||
| package datastoreutils | |||
There was a problem hiding this comment.
Why not in datastore package directly?
There was a problem hiding this comment.
Because that would create a loop: flags refrence datastore, and we need flags there :/
| return err | ||
| } | ||
|
|
||
| func Dial(ctx context.Context, logger *zap.Logger, withCheckCron bool) (*Store, error) { |
There was a problem hiding this comment.
Inline those Dial functions?
There was a problem hiding this comment.
I'm not sure how and why, what do you have in mind?
| } | ||
| } | ||
|
|
||
| func DialStore[S any](ctx context.Context, dbName string, withCheckCron bool, newStore func(*datastore.Datastore) (S, error)) (S, error) { |
There was a problem hiding this comment.
In the end, do we ever create stores without dialing them? If not, collapse DialStore and NewStore? (at least for what is exposed outside the package)
There was a problem hiding this comment.
Tests does, for now, as they want to override the clock.
We could add it as parameters, but that would be future PR no?
| } | ||
| s, err := newStore(db) | ||
| if err != nil { | ||
| db.Pool.Close() |
There was a problem hiding this comment.
Are you sure this is safe to call here? (before it was within the next if block)
-> fix in a separate PR if it is
| connectParameters := flags.ConnectParameters() | ||
| connectParameters.DBName = "aux" | ||
| datastore, err := datastore.Dial(ctx, connectParameters) | ||
| auxStore, err := auxc.Dial(ctx, logger, true) |
There was a problem hiding this comment.
The withCronCheck should be false here
There was a problem hiding this comment.
Are you sure? I would normalize scd/rid/aux there, no? Why aux wouldn't be checked like the others?
| if withCheckCron { | ||
| c := cron.New() | ||
| if _, err := c.AddFunc("@every 1m", func() { checkDatabase(ctx, db, dbName) }); err != nil { | ||
| db.Pool.Close() |
There was a problem hiding this comment.
This was not here before, is this safe to call?
-> fix in a separate PR if it is
There was a problem hiding this comment.
Before it was only called in one return, so I normalized it. Since the db is going to be unused anymore, I do think it's ok and make sense to properly clean up in all cases.
| scdV1Server, err = createSCDServer(ctx, logger) | ||
| if err != nil { | ||
| ridV1Server.Cron.Stop() | ||
| ridV2Server.Cron.Stop() |
2 participants
Follow #1408 , extracted from #1403
Move to a generic
dialfunction, normalizingdatastore.Dialcalls, checks of errors and (optional) cron creation.