refactor: Make rootstore require Batching and TxnDatastore #940

fredcarle · 2022-11-13T20:27:45Z

Relevant issue(s)

Resolves #683

Description

This PR change the requirement for our supported datastore to implement both ds.Batching and ds.TxnDatastore. This means that the map datastore is no longer supported.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

make test

Specify the platform(s) on which this was tested:

MacOS

fredcarle · 2022-11-13T20:30:00Z

db/fetcher/versioned.go

@@ -431,7 +420,6 @@ func (vf *VersionedFetcher) getDAGNode(c cid.Cid) (*dag.ProtoNode, error) {
 }

 func (vf *VersionedFetcher) Close() error {
-	vf.store.Discard(vf.ctx)
 	if err := vf.root.Close(); err != nil {
 		return err
 	}


thought: I'm not quite sure if there are any negative impact to that change. What I do know is that keeping it there cause a Unclosed iterator at time of Txn.Discard error.

I think its OK to remove this, likely here accidentally since the vf.store is a transaction that isn't "owned" by the VersionFetcher, so I don't think it should be responsbile to Discard it.

I am intrigued why its suddenly causing this issue.

It did this when trying to switch to a transactional datastore when using the old map db. If I add it back now it works fine. I'll leave it out though as like you mentioned, it was probably added there by accident.

db/fetcher/versioned.go

AndrewSisley

Code change is simple and I am happy with it, but this is a new restriction on Defra and should be approved by John (product-decision, not technical)

codecov · 2022-12-16T05:44:09Z

Codecov Report

Merging #940 (829ad47) into develop (2e5626f) will increase coverage by 0.04%.
The diff coverage is 96.29%.

@@             Coverage Diff             @@
##           develop     #940      +/-   ##
===========================================
+ Coverage    57.73%   57.77%   +0.04%     
===========================================
  Files          171      172       +1     
  Lines        19478    19455      -23     
===========================================
- Hits         11246    11241       -5     
+ Misses        7239     7225      -14     
+ Partials       993      989       -4

Impacted Files	Coverage Δ
cli/serverdump.go	`15.78% <0.00%> (ø)`
db/collection.go	`66.61% <ø> (+0.40%)`	⬆️
db/collection_update.go	`59.01% <ø> (+0.54%)`	⬆️
cli/start.go	`43.44% <100.00%> (ø)`
datastore/memory/errors.go	`100.00% <100.00%> (ø)`
datastore/memory/memory.go	`88.73% <100.00%> (ø)`
datastore/memory/txn.go	`88.88% <100.00%> (-0.14%)`	⬇️
datastore/txn.go	`62.06% <100.00%> (-1.01%)`	⬇️
datastore/wrappedStore.go	`66.01% <100.00%> (ø)`
db/db.go	`73.00% <100.00%> (ø)`
... and 2 more

jsimnz

LGTM.

Some small notes/questions. Most notably in the db/db_test.go we prob want to keep the badger inmemory store for those tests (or both)

db/db_test.go

jsimnz · 2022-12-16T12:38:25Z

db/fetcher/versioned.go

@@ -431,7 +420,6 @@ func (vf *VersionedFetcher) getDAGNode(c cid.Cid) (*dag.ProtoNode, error) {
 }

 func (vf *VersionedFetcher) Close() error {
-	vf.store.Discard(vf.ctx)
 	if err := vf.root.Close(); err != nil {
 		return err
 	}


I think its OK to remove this, likely here accidentally since the vf.store is a transaction that isn't "owned" by the VersionFetcher, so I don't think it should be responsbile to Discard it.

I am intrigued why its suddenly causing this issue.

datastore/txn.go

db/fetcher/versioned.go

AndrewSisley

Got to run, so submitting a few comments now

cli/start.go

AndrewSisley · 2022-12-16T15:55:11Z

datastore/memory/txn.go

@@ -208,20 +208,19 @@ func (t *basicTxn) Commit(ctx context.Context) error {
 	if t.discarded {
 		return ErrTxnDiscarded
 	}
-	if t.readOnly {
-		return ErrReadOnlyTxn


question: My brain is blanking here now - why are you removing this error?

Because our collection api does a commit (c.commitImplicitTxn) on Get which is probably used to discard the transaction but nonetheless expects no errors of committing a read only transaction. Committing on a read only transaction now simply discards the transaction without error.

Do we have any (other) protection against mutations made during a read only transaction?

The actual Put method checks if its readonly before doing the operation.

The commitImplicitTxn is unrelated to this kind of stuff tho. Not sure how I missed this line, its def not an error to Commit a readonly transaction, its just basically a no-op.

datastore/txn.go

AndrewSisley

Looks good! I really think DEFRA_IN_MEMORY needs to be added to the CI tests though

db/db_test.go

AndrewSisley · 2022-12-16T16:02:41Z

tests/integration/mutation/relation/create/with_txn_test.go

@@ -171,8 +171,6 @@ func TestTransactionalCreationAndLinkingOfRelationalDocumentsForward(t *testing.
 				},
 			},
 		},
-
-		DisableMapStore: true,


praise: Didnt realise we had so many of these now 😅 Really nice to lose them

tests/integration/utils.go

Height is thread safe and Len isn’t.

…work#940) Relevant issue(s) Resolves sourcenetwork#683 Description This PR change the requirement for our supported datastore to implement both ds.Batching and ds.TxnDatastore. This means that the map datastore is no longer supported.

fredcarle added area/datastore Related to the datastore / storage engine system action/no-benchmark Skips the action that runs the benchmark. labels Nov 13, 2022

fredcarle added this to the DefraDB v0.4 milestone Nov 13, 2022

fredcarle requested a review from a team November 13, 2022 20:27

fredcarle self-assigned this Nov 13, 2022

fredcarle commented Nov 13, 2022

View reviewed changes

fredcarle changed the title ~~refactor: Make our rootstore require Batching and TxnDatastore~~ refactor: Make rootstore require Batching and TxnDatastore Nov 13, 2022

fredcarle commented Nov 16, 2022

View reviewed changes

db/fetcher/versioned.go Outdated Show resolved Hide resolved

jsimnz mentioned this pull request Nov 17, 2022

feat: Add thread safe transactional in-memory datastore #947

Merged

4 tasks

AndrewSisley reviewed Nov 21, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/refactor/I683-remove-batchable branch from f60e17c to 01ffc86 Compare December 16, 2022 05:40

jsimnz approved these changes Dec 16, 2022

View reviewed changes

AndrewSisley reviewed Dec 16, 2022

View reviewed changes

AndrewSisley requested changes Dec 16, 2022

View reviewed changes

fredcarle force-pushed the fredcarle/refactor/I683-remove-batchable branch from f05a856 to dea44f8 Compare December 16, 2022 16:35

fredcarle added 8 commits December 18, 2022 21:58

remove map datastore support

59306f8

change to defra in-memory datastore

e4c0d54

change from badger in-memory to defra in-memory

48cc9bf

remove deadcode

d55467a

revert to badger in-memory

e72eb68

apply feedback

e619afd

revert removing txn shim

ede4022

add custom txn conflict error

4df2b24

fredcarle force-pushed the fredcarle/refactor/I683-remove-batchable branch from 7617f37 to 4df2b24 Compare December 19, 2022 02:58

fix btree len to height

829ad47

Height is thread safe and Len isn’t.

AndrewSisley approved these changes Dec 19, 2022

View reviewed changes

fredcarle merged commit 664d510 into develop Dec 19, 2022

fredcarle deleted the fredcarle/refactor/I683-remove-batchable branch December 19, 2022 19:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Make rootstore require Batching and TxnDatastore #940

refactor: Make rootstore require Batching and TxnDatastore #940

fredcarle commented Nov 13, 2022

fredcarle Nov 13, 2022

jsimnz Dec 16, 2022

fredcarle Dec 16, 2022

AndrewSisley left a comment

codecov bot commented Dec 16, 2022 •

edited

Loading

jsimnz left a comment

jsimnz Dec 16, 2022

AndrewSisley left a comment

AndrewSisley Dec 16, 2022

fredcarle Dec 16, 2022

AndrewSisley Dec 16, 2022

jsimnz Dec 16, 2022

AndrewSisley left a comment

AndrewSisley Dec 16, 2022

fredcarle Dec 16, 2022

jsimnz Dec 16, 2022

refactor: Make rootstore require Batching and TxnDatastore #940

refactor: Make rootstore require Batching and TxnDatastore #940

Conversation

fredcarle commented Nov 13, 2022

Relevant issue(s)

Description

Tasks

How has this been tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 16, 2022 • edited Loading

Codecov Report

jsimnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 16, 2022 •

edited

Loading