Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Restructure db/txn/multistore structures #199

Merged
merged 1 commit into from
Feb 14, 2022

Conversation

AndrewSisley
Copy link
Contributor

Closes #172

  • Removes the multi-store-properties declaration duplication
  • Merges the two transaction interfaces and implementations
  • Removes non-datastore access from transactions
  • Moves Commit/Discard onto the core transaction (removed from iterable)
  • Reworks the shimming of batchable => transaction stores, and removes the related code duplication in db/txn
  • Reworks the shimming of (non)iterable transaction stores, and removes the related code duplication in db/txn
  • DocFetcher now uses the datastore (instead of root)

@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #199 (79f92b4) into develop (8511a1c) will decrease coverage by 0.56%.
The diff coverage is 71.37%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #199      +/-   ##
===========================================
- Coverage    57.52%   56.96%   -0.57%     
===========================================
  Files           98       99       +1     
  Lines         9616     9664      +48     
===========================================
- Hits          5532     5505      -27     
- Misses        3463     3547      +84     
+ Partials       621      612       -9     
Impacted Files Coverage Δ
db/blockstore.go 0.00% <0.00%> (ø)
db/schema.go 38.63% <0.00%> (ø)
db/collection_update.go 41.40% <50.00%> (ø)
db/sequence.go 65.11% <50.00%> (ø)
store/wrappedStore.go 61.29% <61.29%> (ø)
query/graphql/planner/executor.go 79.06% <66.66%> (ø)
store/txn.go 61.42% <72.72%> (+4.28%) ⬆️
db/collection_delete.go 28.07% <77.77%> (ø)
store/shimstore.go 77.77% <77.77%> (ø)
db/db.go 53.98% <82.35%> (-0.49%) ⬇️
... and 14 more

Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was concerend when I initial saw the # of lines/files changed, but I can see now its mostly updating all the references to core.Txn and the utility funcs for the stores.

In general I think this is much more organized. I had a question about the Copy method, and re-implementing the namespace package.

My only comment is whether it makes more sense to have the Txn interface defined in core or client as per our convo in discord. I personally think it belongs with the rest of the interfaces in the client package.

core/store.go Outdated Show resolved Hide resolved
db/blockstore.go Outdated Show resolved Hide resolved
store/wrappedStore.go Show resolved Hide resolved
store/wrappedStore.go Show resolved Hide resolved
@AndrewSisley
Copy link
Contributor Author

Was concerend when I initial saw the # of lines/files changed, but I can see now its mostly updating all the references to core.Txn and the utility funcs for the stores.

In general I think this is much more organized. I had a question about the Copy method, and re-implementing the namespace package.

My only comment is whether it makes more sense to have the Txn interface defined in core or client as per our convo in discord. I personally think it belongs with the rest of the interfaces in the client package.

Would like to sort out the client vs core issue in #200 if alright with you?

@AndrewSisley AndrewSisley force-pushed the sisley/refactor/I172-txn-refac branch 2 times, most recently from 470c821 to e85c979 Compare February 11, 2022 14:45
@jsimnz
Copy link
Member

jsimnz commented Feb 11, 2022

Would like to sort out the client vs core issue in #200 if alright with you?

As you you'd prefer to merge this now as is with Txn defined in core/ then address the whole relation/naming/organization of core/ vs client/ in #200?

Thats prob fine :)

@AndrewSisley
Copy link
Contributor Author

Would like to sort out the client vs core issue in #200 if alright with you?

As you you'd prefer to merge this now as is with Txn defined in core/ then address the whole relation/naming/organization of core/ vs client/ in #200?

Thats prob fine :)

Yeah sorry - my English was horrible in my earlier reply - would rather do it then, might get a bit involved as might want to make sure all the stuff on the Txn interface is in client too (or whatever we decide in that ticket)

@AndrewSisley AndrewSisley self-assigned this Feb 14, 2022
Copy link
Member

@jsimnz jsimnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After final reivew, all is good!

LGTM

@AndrewSisley
Copy link
Contributor Author

Sweet - cheers

@AndrewSisley AndrewSisley merged commit cdaad72 into develop Feb 14, 2022
@AndrewSisley AndrewSisley deleted the sisley/refactor/I172-txn-refac branch February 14, 2022 22:47
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hefty disjoint between txn.go.newTxn/Txn and db.go.NewDB/DB
2 participants