refactor(i): Remove datastore.Txn from private db methods #2518
Conversation
nasdf
added
the
area/db-system
nasdf
changed the title
| @@ -53,6 +53,11 @@ func ensureContextTxn(ctx context.Context, db transactionDB, readOnly bool) (con | |||
| return SetContextTxn(ctx, txn), txn, nil | |||
| } | |||
|
|
|||
| // mustGetContextTxn returns the transaction from the context or panics. | |||
| func mustGetContextTxn(ctx context.Context) datastore.Txn { | |||
| return ctx.Value(txnContextKey{}).(datastore.Txn) | |||
There was a problem hiding this comment.
todo: This is a user input, and I think an error if not found would be much better than a panic - unless it is always guaranteed to exist at the point that this function is called, in which case please document this in this function.
There was a problem hiding this comment.
I've updated the docs to better reflect our usage pattern. It should be guaranteed to exist on any private function with db.
There was a problem hiding this comment.
Nice - new doc changes look good, thanks Keenan :)
| @@ -53,6 +53,11 @@ func ensureContextTxn(ctx context.Context, db transactionDB, readOnly bool) (con | |||
| return SetContextTxn(ctx, txn), txn, nil | |||
| } | |||
|
|
|||
| // mustGetContextTxn returns the transaction from the context or panics. | |||
| func mustGetContextTxn(ctx context.Context) datastore.Txn { | |||
There was a problem hiding this comment.
suggestion: This feels like an odd place for such a function, why is this defined privately in db, instead of internal-public in core? Is the long term plan to move it, or to duplicate it? Or have we managed to remove all txn usage from all other packages?
There was a problem hiding this comment.
From the standup discussion, our consensus was to use this only within the db package. Other APIs will still use txn params. I agree that it should move to core if we decide to expand it to other areas.
| @@ -53,6 +53,11 @@ func ensureContextTxn(ctx context.Context, db transactionDB, readOnly bool) (con | |||
| return SetContextTxn(ctx, txn), txn, nil | |||
There was a problem hiding this comment.
suggestion: Sorry I missed the PR in which this was added, would you mind noting that a new context.Context object may be returned from this function (in the case of implicit txn)? It is not clear from the existing documentation.
Something along the lines of:
... otherwise a new implicit transaction will be created, and returned within a new context object also containing the copied values from the input context.
I know this is expected with Go contexts, but it is important and thus worth being nice and explicit here IMO.
There was a problem hiding this comment.
Just adding some context here ;). A new context.Context will always be returned regardless of whether the implicit txn is created. Since the existing txn on the context is converted to an explicityTxn and then re-added to the context, thus creating a new context (since they are immutable).
With the exception of the error path
Relevant issue(s)
Resolves #2517
Description
This PR removes the
datastore.Txnparameter from private db methods.Tasks
How has this been tested?
make testSpecify the platform(s) on which this was tested: