refactor: Reorganise client transaction related interfaces #1180
Conversation
AndrewSisley
added
area/api
Codecov Report
@@ Coverage Diff @@
## develop #1180 +/- ##
===========================================
- Coverage 68.77% 68.64% -0.14%
===========================================
Files 180 181 +1
Lines 17046 17126 +80
===========================================
+ Hits 11724 11756 +32
- Misses 4370 4415 +45
- Partials 952 955 +3
|
There was a problem hiding this comment.
I'm still trying to wrap my head around the idea of having *db embedded in a struct with nothing else for the purpose of scoping methods. At the moment I'm not convinced so I'll wait to see others' opinions.
I think we definitely need to improve the naming of the two new structs and the file that contains them and ensure it describes the content.
On the issue that you raise about planner.New, it can be easily fixed if WithTxn returned a TxnStore type that looked like this
type TxnStore interface {
Store
datastore.Txn
}as well as changing explicitDB to
type explicitDB struct {
*db
datastore.Txn
}with a few more trivial changes.
db/implicit.go
Outdated
| type implicitDb struct { | ||
| *db | ||
| } | ||
|
|
||
| type explicitDb struct { | ||
| *db | ||
| txn datastore.Txn | ||
| } |
There was a problem hiding this comment.
todo: implicitDB and explicitDB are misleading names. It isn't the db that is implicit or explicit but rather it is the transaction. implicitTxn and explicitTxn would be more appropriate. If you don't like those two I'm open to suggestions.
There was a problem hiding this comment.
I agree with @fredcarle here that names seem misleading. Perhaps there is a way to avoid the use of implicit and explicit in filename and in source? don't have any super nice suggestions atm, maybe Db and DbTxn?
There was a problem hiding this comment.
The problem I see is that both are transactional, and both are databases. implicit is still 100% transactional (bugs aside...), it is just not an externally exposed transaction.
Accounting for the fact that these types are internal only, and so we are only ever talking about internal 'stuff', one could actually argue that implicit is more transactional than explicit, as the volume of code relating to transactions is actually higher (in implicit compared to explicit).
There was a problem hiding this comment.
Discussed in discord, will rename these to implicitTxnDB and explicitTxnDB
- Rename types (dont forget
DBwith two capitals...)
db/implicit.go
Outdated
There was a problem hiding this comment.
suggestion: The name of this file I find is at the same time to vague and too narrow for what it contains. To content could be put in db.go instead but if you really want it in it's own file, maybe we could try to find a better name for it.
There was a problem hiding this comment.
Discussed in discord, will rename to txn_db.go
- Rename file
AndrewSisley
force-pushed
the
sisley/refactor/I1161-client-txnality-2
branch
from
5774180
to
20caa3e
Compare
Store will form the basis of the new transactional split, and will be the type returned from the soon-to-be WithTxn func
* Extract Store functions to new interface Store will form the basis of the new transactional split, and will be the type returned from the soon-to-be WithTxn func * Wrap transactionality using WithTxn
…work#1180) * Extract Store functions to new interface Store will form the basis of the new transactional split, and will be the type returned from the soon-to-be WithTxn func * Wrap transactionality using WithTxn
Relevant issue(s)
Resolves #1161
Description
Reorganises the client transaction related interfaces so that we are not doubling up the visible function count with
Txnsuffix variants.Partly designed via discord thread
dev-db.Client interface refactorhttps://discord.com/channels/427944769851752448/1083107751435173918Out of scope
dbin both explicit and implicit implementations. Relative to the other stuff I need to do before release, I just don't care enough about this. I'm moderately happy with the current design and think it is good enough. I really don't like the proposed idea of havingdbandcoreDb, or directly exposingdband dropping either the explicit or implicit implementation, or a circular reference variant that relies on runtime checks. If people really want one of those, I suggest it is done later, by someone who thinks it is a nice change. This PR deals with the important externally visible stuff, and that is important for 0.5, the internals are less important, and it makes no sense to have me implement something I think is a bad idea.txnandclient.Storeare required for some use cases. I find that really really horrible -planneris a good example (highly visible viaplanner.New). We have two objects that represent the same transaction, yet that linkage is hidden and it makes it look like theStoreis not respecting the siblingtxnvariable. I really really want to moveMultiStoreontoStoreso that we don't have to do this, but I see that as out of scope for now, and whilst it is horrible IMO, Schema Updates is still more important.clientthat are affected by this PR is non-existent, I see fixing that as out of scope. Documentation that did exist, should however still exist, and should not be degraded by this PR.