From 59bac0f99d0680409b53cd36655efd3df5a5bad4 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 2 Jun 2020 16:05:08 -0400 Subject: [PATCH] state: Update docstrings for changeTrackerDB and txn And un-embed memdb.DB to prevent accidental access to underlying methods. --- agent/consul/state/memdb_wrapper.go | 83 ++++++++++++++++------------- agent/consul/state/state_store.go | 2 +- 2 files changed, 46 insertions(+), 39 deletions(-) diff --git a/agent/consul/state/memdb_wrapper.go b/agent/consul/state/memdb_wrapper.go index fa510370f5..cfa0636416 100644 --- a/agent/consul/state/memdb_wrapper.go +++ b/agent/consul/state/memdb_wrapper.go @@ -4,41 +4,45 @@ import ( "github.com/hashicorp/go-memdb" ) -// memDBWrapper is a thin shim over memdb.DB which forces all new tranactions to -// report changes and for those changes to automatically deliver the changed -// objects to our central event handler in case new streaming events need to be -// omitted. +// memDBWrapper is a thin wrapper around memdb.DB which enables TrackChanges on +// all write transactions. When the transaction is committed the changes are +// sent to the eventPublisher which will create and emit change events. type memDBWrapper struct { - *memdb.MemDB + db *memdb.MemDB // TODO: add publisher } -// Txn intercepts MemDB.Txn(). It allows read-only transactions to pass through -// but fails write transactions as we now need to force callers to use a -// slightly different API so that change capture can happen automatically. The -// returned Txn object is wrapped with a no-op wrapper that just keeps all -// transactions in our state store the same type. The wrapper only has -// non-passthrough behavior for write transactions though. +// Txn exists to maintain backwards compatibility with memdb.DB.Txn. Preexisting +// code may use it to create a read-only transaction, but it will panic if called +// with write=true. +// +// Deprecated: use either ReadTxn, or WriteTxn. func (db *memDBWrapper) Txn(write bool) *txnWrapper { if write { panic("don't use db.Txn(true), use db.WriteTxn(idx uin64)") } - return &txnWrapper{ - Txn: db.MemDB.Txn(false), - } + return db.ReadTxn() +} + +// ReadTxn returns a read-only transaction which behaves exactly the same as +// memdb.Txn +func (db *memDBWrapper) ReadTxn() *txnWrapper { + return &txnWrapper{Txn: db.db.Txn(false)} } // WriteTxn returns a wrapped memdb.Txn suitable for writes to the state store. -// It will track changes and publish events for them automatically when Commit -// is called. The idx argument should be the index of the currently operating -// Raft operation. Almost all mutations to state should happen as part of a raft -// apply so the index of that log being applied should be plumbed through to -// here. A few exceptional cases are transactions that are only executed on a -// fresh memdb store during a Restore and a few places in tests where we insert +// It will track changes and publish events for the changes when Commit +// is called. +// +// The idx argument must be the index of the current Raft operation. Almost +// all mutations to state should happen as part of a raft apply so the index of +// the log being applied can be passed to WriteTxn. +// The exceptional cases are transactions that are executed on an empty +// memdb.DB as part of Restore, and those executed by tests where we insert // data directly into the DB. These cases may use WriteTxnRestore. func (db *memDBWrapper) WriteTxn(idx uint64) *txnWrapper { t := &txnWrapper{ - Txn: db.MemDB.Txn(true), + Txn: db.db.Txn(true), Index: idx, } t.Txn.TrackChanges() @@ -47,40 +51,43 @@ func (db *memDBWrapper) WriteTxn(idx uint64) *txnWrapper { // WriteTxnRestore returns a wrapped RW transaction that does NOT have change // tracking enabled. This should only be used in Restore where we need to -// replace the entire contents of the Store without a need to track the changes -// made and emit events. Subscribers will all reset on a restore and start -// again. It also uses a zero index since the whole restore doesn't really occur +// replace the entire contents of the Store without a need to track the changes. +// WriteTxnRestore uses a zero index since the whole restore doesn't really occur // at one index - the effect is to write many values that were previously // written across many indexes. func (db *memDBWrapper) WriteTxnRestore() *txnWrapper { t := &txnWrapper{ - Txn: db.MemDB.Txn(true), + Txn: db.db.Txn(true), Index: 0, } return t } -// txnWrapper wraps a memdb.Txn to automatically capture changes and process -// events for the write as it commits. This can't be done just with txn.Defer -// because errors the callback is invoked after commit completes so errors -// during event publishing would cause silent dropped events while the state -// store still changed and the write looked successful from the outside. +// txnWrapper wraps a memdb.Txn to capture changes and send them to the +// EventPublisher. +// +// This can not be done with txn.Defer because the callback passed to Defer is +// invoked after commit completes, and because the callback can not return an +// error. Any errors from the callback would be lost, which would result in a +// missing change event, even though the state store had changed. type txnWrapper struct { - // Index stores the index the write is occuring at in raft if this is a write - // transaction. If it's zero it means this is a read transaction. + // Index in raft where the write is occurring. The value is zero for a + // read-only transaction, and for a WriteTxnRestore transaction. + // Index is stored so that it may be passed along to any subscribers as part + // of a change event. Index uint64 *memdb.Txn - - store *Store } -// Commit overrides Commit on the underlying memdb.Txn and causes events to be -// published for any changes. Note that it has a different signature to -// memdb.Txn - returning an error that should be checked since an error implies -// that something prevented the commit from completing. +// Commit first pushes changes to EventPublisher, then calls Commit on the +// underlying transaction. // +// Note that this function, unlike memdb.Txn, returns an error which must be checked +// by the caller. A non-nil error indicates that a commit failed and was not +// applied. // TODO: currently none of the callers check error, should they all be changed? func (tx *txnWrapper) Commit() error { + // changes may be empty if this is a read-only or WriteTxnRestore transaction. //changes := tx.Txn.Changes() // TODO: publish changes diff --git a/agent/consul/state/state_store.go b/agent/consul/state/state_store.go index 93086a4886..758b67bc36 100644 --- a/agent/consul/state/state_store.go +++ b/agent/consul/state/state_store.go @@ -161,7 +161,7 @@ func NewStateStore(gc *TombstoneGC) (*Store, error) { lockDelay: NewDelay(), } s.db = &memDBWrapper{ - MemDB: db, + db: db, } return s, nil }