TTL + table store

[cody-littley]

Why are these changes needed?

This PR unifies the concepts of a "table store" and a "ttl store". Table stores now support TTLs, and there is no stand alone ttl store.

Checks

• I've made sure the lint is passing in this PR.
• I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
• I've checked the new test coverage and the coverage percentage didn't drop.
• Testing Strategy
  • Unit tests
  • Integration tests
  • This PR is not tested :(

[jianoaix]

Table view tries to appear like a store, by wrapping two stores...these are getting too complicated and won't be simple to use.

[cody-littley]

This isn't actually wrapping two stores, this class has access to two different interfaces on the same store. This constructor isn't publicly exported, so it's impossible for an end user to instantiate this class with two different stores.

That being said, in order to simplify this method, I've modified the function signature to make things easier to read.

The reason why this class needed access to the parent TableStore class was for the methods Shutdown(), Destroy(), and NewBatch(). Instead of passing in the whole TableStore, we can just pass in lambdas for the functionality needed by the table view.

func newTableView(
	base kvstore.Store,
	name string,
	prefix uint32,
	shutdown func() error,
	destroy func() error,
	batchBuilder func() kvstore.TableStoreBatch) kvstore.Table {

[jianoaix]

The specific issue here is that tableView doesn't appear to be Store, it's trying to be a Table (e.g. the TTL related interfaces do not exist for Store)

[cody-littley]

The interface Table currently extends the interface Store. A user can interact with a table as if it were a regular Store object.

type Table interface {
	Store

	Name() string
	TableKey(key []byte) TableKey
	PutWithTTL(key []byte, value []byte, ttl time.Duration) error
	PutWithExpiration(key []byte, value []byte, expiryTime time.Time) error
	NewTTLBatch() TTLStoreBatch
}

[jianoaix]

since you are trying to merge ttl into tablestore, can these be deleted?

[cody-littley]

Done. I've merged the TTLStore interface into the Table interface.

[jianoaix]

nit: createTable

[cody-littley]

fixed

[jianoaix]

It looks this is not needed?

• if it's a Table, there is TableBatch, operating on the raw keys
• if it's a TableStore, there is TableStoreBatch, operating on the namespaced keys

And the ttl related interfaces are always there.

[cody-littley]

The batch APIs have now been simplified so that they don't have generics. Are there additional simplifications you'd like to see?

[cody-littley]

Note that we still need two batch interfaces if we want to support batches that can set different TTLs for operations within the same batch. The core reason is that the basic Store API does not have a concept of a TTL, meaning we need a batch interface that doesn't have TTLs.

[jianoaix]

The specific issue here is that tableView doesn't appear to be Store, it's trying to be a Table (e.g. the TTL related interfaces do not exist for Store)

[jianoaix]

Why the following 3 interfaces make sense to be here? They are all operating as the TableStore, not Table

[cody-littley]

The core problem is that Table implements Store, which means it needs to implement methods like Shutdown(). If you'd like to change this, I can think of the following options:

• make methods like Shutdown() into no-ops
• make methods like Shutdown() return an error
• split the Store interface into several interfaces:
  • the interface called Store will just have the methods Get(), Put(), Delete(), etc.
  • There will be an interface called DBStore (or similar) that implements Store and adds methods such as Shutdown()
  • Things such as LevelDBStore will implement DBStore, while Table will only implement Store. Table will no longer have odd methods such as Shutdown().
• Replace Tables with key builders (I don't think you like this option, mentioning it for completeness 😜)

[jianoaix]

I'm open to either option if it makes sense. I think the "weirdness" in APIs may indicate we could structure them better (like the case here). Do you prefer using key builder? Option 3 seems complicated to me.

[jianoaix]

lgtm mostly

[jianoaix]

These are a lot of types, can we start with what we know we need?

[cody-littley]

I've removed all of the methods except for the ones that deal with bytes. The other methods were not strictly necessary, just syntactic sugar.