Implement new TxDatastore and Txn interfaces #27
Conversation
|
i could also bundle the TTL implementation in this branch which, thinking about it, seems to make the most sense as it will eventually depend on this code, too! |
|
added TTL! |
bigs
changed the title
|
i'd say this is ready for review now. @Stebalien not sure who else to rope in, but this is relevant to stuff we've been thinking about |
| return true, nil | ||
| } | ||
| func (t *txn) Has(key ds.Key) (bool, error) { | ||
| _, err := t.Get(key) |
There was a problem hiding this comment.
Is there still no way to do that touching vlogs / having to fetch the value?
There was a problem hiding this comment.
ah you know what i can open a transaction that specifically doesn't pre-load values. i will change that. good catch
datastore.go
Outdated
| err = nil | ||
| } | ||
| return err | ||
| func (t *txn) Rollback() { |
There was a problem hiding this comment.
imo this name is rather confusing as at least in the SQL world it usually means 'undo changes by this transaction'
There was a problem hiding this comment.
hmm ok, i can work w/ that. perhaps we go with Discard?
There was a problem hiding this comment.
Makes more sense, but still deserves a docstring explaining that it doesn't touch committed data
There was a problem hiding this comment.
+1 on Discard, especially because the user will see it being called on every txn method.
ds_test.go
Outdated
| @@ -547,3 +548,155 @@ func TestDiskUsage(t *testing.T) { | |||
| } | |||
| d.Close() | |||
| } | |||
|
|
|||
| func TestTxnRollback(t *testing.T) { | |||
| d, err := NewDatastore("/tmp/testing_badger_du", nil) | |||
datastore.go
Outdated
| @@ -18,6 +19,10 @@ type datastore struct { | |||
| gcDiscardRatio float64 | |||
| } | |||
|
|
|||
| type txn struct { | |||
There was a problem hiding this comment.
Could we add here that this is documenting the datastore.Txn interface?
datastore.go
Outdated
| err = nil | ||
| } | ||
| return err | ||
| func (t *txn) Rollback() { |
There was a problem hiding this comment.
+1 on Discard, especially because the user will see it being called on every txn method.
There was a problem hiding this comment.
This LGTM but I feel we're abusing the Datastore interface a bit with the new Txn that extends it.
Although the signature of the functions are the same their semantic is different in the key aspect that Txn requires Commit() to be called (it's not an optional extension) to fulfill the Put() promise. A new user that just follows a Get/Put definition all the way to Datastore won't realize that difference.
|
@schomatis yeah, i do understand that. i could just duplicate the method signatures in Txn if that would help. either way, until we come to a decision on that i'll make sure the interface has good docstrings to explain. ultimately, i think it'd be a mistake not to implement transactions in their fullest capacity just because golang makes it difficult to represent the types well. there's an awful lot of performance/efficiency to be gained! |
Agreed. |
|
updated, rebased |
|
@schomatis yeahhhhhhhh. what do you think of explicitly moving datastore operations onto the txn interface? i think that would at least make it clear that there is a separation. |
|
cheers, thanks for feedback @schomatis ! change with that is up on the datastore repo |
ghost
assigned 
adds full on transaction support for the badger datastore. related to ipfs/go-datastore#91, though not dependent on it. also updates all vanilla Datastore methods to leverage the transaction wrapper.
WIP for now because it doesn't include transaction specific tests, though all existing tests (which depend on them implicitly) are passing.