Snake a context through the Chain-blockstore creation #5282
Conversation
ribasushi
requested review from
Kubuxu,
magik6k and
whyrusleeping
as code owners
Co-authored-by: raulk <raul@protocol.ai>
There was a problem hiding this comment.
The interfaces are levelled out now. I'm good to merge. Thanks @ribasushi.
|
ping @magik6k for final call / merge. |
raulk
force-pushed
the
chore/snake_context_through_blockstore_init
branch
from
a59177f
to
a1da1da
Compare
|
Depends on #4995 to fix the build-lotus-soup CI job. |
| ds, err := r.Datastore("/staging") | ||
| func StagingMultiDatastore(lc fx.Lifecycle, mctx helpers.MetricsCtx, r repo.LockedRepo) (dtypes.StagingMultiDstore, error) { | ||
| ctx := helpers.LifecycleCtx(mctx, lc) | ||
| ds, err := r.Datastore(ctx, "/staging") |
There was a problem hiding this comment.
Doc for the Datastore says that ctx is only for startup, here we are passing a lifecycle context from start to the end of Lotus runtime.
There was a problem hiding this comment.
The Datastore() constructor should not retain the context. It should only be used for the start operation, and this is the only context that we have that is cancelled if the startup is cancelled.
There was a problem hiding this comment.
Hmm, we should be using the context from OnStart here, but it makes everything way harder.
There was a problem hiding this comment.
Yeah, that would be unnecessarily complex IMO.
|
Approved, but #4995 needs to land first for the tests to pass. |
…snake_context_through_blockstore_init Snake a context through the Chain-blockstore creation
This sends through the startup daemon context for alternative blockstores that could benefit from it.
EDIT (by @raulk)
Context: one datastore that will benefit from this is the Postgres-backed datastore. Since it is not an embedded datastore and needs to set up a network connection on initialisation, a context is tremendously useful.