Add TTL and Transactional interfaces #91
Conversation
datastore.go
Outdated
| } | ||
|
|
||
| // Txn is an interface for transactions that can be committed or rolled back. | ||
| type Txn interface { |
There was a problem hiding this comment.
Maybe just Tx since the datastore is TxDatastore?
There was a problem hiding this comment.
went the other way, but made them consistent!
datastore.go
Outdated
|
|
||
| // Txn is an interface for transactions that can be committed or rolled back. | ||
| type Txn interface { | ||
| Datastore |
There was a problem hiding this comment.
Not sure if we want to say that txn should implement stuff like Query. Feels a bit badger specific.
There was a problem hiding this comment.
the reasoning here is that updating values (i.e. reading an existing value and manipulating it) is pretty essential to most transactional behavior. without query support you just have the existing Batching interface.
ghost
assigned 
|
renamed Rollback -> Discard and made some docstrings |
datastore.go
Outdated
| // queries and mutations to the Datastore into atomic groups, or transactions. | ||
| // Actions performed on a transaction will not take hold until a successful | ||
| // call to Commit has been made. Likewise, transactions can be aborted by | ||
| // calling Discard before a successful Commit has been made. |
There was a problem hiding this comment.
This comment should be above the type Txn interface line.
There was a problem hiding this comment.
Yes, and maybe this could be named (edit: that already exists.)TxnDatastore.
datastore.go
Outdated
| Get(key Key) (value interface{}, err error) | ||
| Has(key Key) (exists bool, err error) | ||
| Delete(key Key) error | ||
| Query(q query.Query) (query.Results, error) |
There was a problem hiding this comment.
I'd rather extend Datastore here so we avoid duplicating these
There was a problem hiding this comment.
so i generally agree, though this change was made in response to discussion happening in the badger impl, specifically right here. what do you think? the idea was to make it clear that this is happening in the Txn not directly on the Datastore. open to either
There was a problem hiding this comment.
The rationale behind this is that Txn is not extending the Datastore interface but is actually redefining it (committing is not an optional new feature, it's mandatory for the Put to take effect). I agree that this solution is also not ideal, seems to me the lesser of the two evils, but I'm okay if you want to use Datastore here.
There was a problem hiding this comment.
further thoughts on this? think we're pretty close!
There was a problem hiding this comment.
Hey @bigs, I think neither @magik6k nor myself have a strong feeling towards any of the proposed solutions, so it's your call, both solutions are fine by me. @Stebalien Do you any preferences here?
datastore.go
Outdated
| // queries and mutations to the Datastore into atomic groups, or transactions. | ||
| // Actions performed on a transaction will not take hold until a successful | ||
| // call to Commit has been made. Likewise, transactions can be aborted by | ||
| // calling Discard before a successful Commit has been made. |
There was a problem hiding this comment.
Yes, and maybe this could be named (edit: that already exists.)TxnDatastore.
datastore.go
Outdated
| Get(key Key) (value interface{}, err error) | ||
| Has(key Key) (exists bool, err error) | ||
| Delete(key Key) error | ||
| Query(q query.Query) (query.Results, error) |
There was a problem hiding this comment.
The rationale behind this is that Txn is not extending the Datastore interface but is actually redefining it (committing is not an optional new feature, it's mandatory for the Put to take effect). I agree that this solution is also not ideal, seems to me the lesser of the two evils, but I'm okay if you want to use Datastore here.
|
OK, i'm going to revert this to extend |
This reverts commit 8cca168.
bigs
force-pushed
the
feat/superclasses
branch
from
e03716c to
7db32f2
Compare
adds two
Datastoresuperclasses,TTLDatastoreandTxDatastore. together, these give us pretty complete access to datastores such as badger.closes #87 and simplifies a slew of other issues as well.