Table store

[cody-littley]

Why are these changes needed?

This PR adds the ability to use tables on top of a store. A very nice to have feature for use cases that want to use multiple logical tables.

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]

Not sure this needs template, Batch may just work with table key

[cody-littley]

Base Store objects also need to support operations using Batch, but the type of these keys is []byte. I think if we want to get rid of the generics, we will need to stop using type TableKey []byte and instead use the []byte type for batch operations on a table. Let's discuss.

[jianoaix]

The issue is this interface intends to be used for TableStore (like it comments out), but it's now got used to batch operations for Store as well.

[jianoaix]

One idea is to subclass this interface with two types, like (demo not real code):

type StoreBatch struct {
        // key type is []byte
	deletes    map[[]byte]struct{}
        ...
}

type TableStoreBatch struct {
        // key type is TableKey
	deletes    map[TableKey]struct{} 
        ...
}

It establishes common interface but users doesn't need to use template. Wdyt?

[jianoaix]

For simplicity, it looks Batch creation can be a function of TableStore

[cody-littley]

NewBatch is also a function on the base Store object, but I agree this interface isn't really necessary. Removed.

[jianoaix]

Comment doesn't match code

[cody-littley]

fixed

[jianoaix]

Comment doesn't match code

[cody-littley]

fixed

[jianoaix]

I'd create a TableStore, instead of asking for an external Store and wrapping it. This way the Store can be internally created and no risk of breaking invariants by accessing it directly outside the TableStore.

[cody-littley]

The purpose of requiring the caller to provide the Store implementation is to support multiple Store implementations. Currently this is used in unit tests, where I sometimes use a store implementation that is built on top of an in memory map. Let's discuss this.

[cody-littley]

Changed based on our slack discussion.

[jianoaix]

I don't see this TableStore is thread-safe (which is required per interface)

[cody-littley]

In the interface, this method is specifically documented not to be thread safe (although the godoc in the implementation doesn't copy the warning). The code to make this thread simple to write, but I'm concerned about adding locks that could slow other operations. Do you think it's worth adding locks so that these methods are safe to call concurrently?

	// GetTable gets the table with the given name. If the table does not exist, it is first created.
	//
	// WARNING: this method is not thread safe with respect to any other methods in this interface or
	// any methods on any Table objects associated with this store.
	GetTable(name string) (Table, error)

[jianoaix]

I think it needs to, otherwise other methods like GetTableCount (which is supposed to be thread safe) won't be safe as they share the states.