database/sinkdb: new package #1192

jbowens · 2017-05-15T21:44:00Z

Split database/raft into two packages, one handling raft-specific logic,
the other exposing a consistent kv store interface.

database/raft manages the raft.Node and exposes http endpoints for
communication between raft nodes.

database/sinkdb implements a consistent kv store on top of
database/raft. For now, committed kv data is stored in a simple protocol
buffer persisted to the filesystem.

tessr · 2017-05-15T21:52:50Z

core/config/config.go

+// storage, the config will be added to sinkdb.
+func Load(ctx context.Context, db pg.DB, sdb *sinkdb.DB) (*Config, error) {
+	c := new(Config)
+	found, err := sdb.Get(ctx, "/core/config", c)


✨ pretty!

tessr · 2017-05-15T22:04:49Z

database/raft/raft.go

 	confState raftpb.ConfState
 	done      bool

 	// Current log position, accessed only from runUpdates goroutine
 	snapIndex uint64
 }

+type State interface {


why is this an interface? is there more than one implementation of this type?

to circumvent a bidirectional dependency. I was hoping that database/raft would only be raft-specific logic and completely ignorant of its use in implementing a kv store, but it needs a way to access and modify the raft state. This interface keeps key-value logic out of the raft package but still lets it interact with the key-value state.

Ah, got it. Clever.

tessr · 2017-05-15T22:20:54Z

database/raft/raft.go

 	}
 }

-func (sv *Service) get(ctx context.Context, key string) ([]byte, error) {
+func (sv *Service) linearizeRead(ctx context.Context) error {


I know this is wordier, but could we call this linearizableRead instead? linearizeRead sounds like we are taking a read and linearizing it, whatever that means.

Oh, wait, I suppose that's actually what we're doing here. nevermind

It could also just be a lowercase version of the public method, so requestRead.

tessr · 2017-05-15T22:26:46Z

database/sinkdb/sinkdb.go

+
+// GetInconsistent performs a non-linearizable read of the provided key.
+// The value may be stale. The read value is unmarshalled into v.
+func (db *DB) GetInconsistent(key string, v proto.Message) (found bool, err error) {


🚲 I prefer the word "stale" to "inconsistent" but 🚲

🚲 I co-opted the terminology from cockroach's consistency levels: cockroachdb/cockroach#343 (comment)

I get the point Spencer's making since in many cases, the value will not actually be stale, but I'm not sure if "inconsistent" is better. ¯\_(ツ)_/¯

🚲 🚲 🚲 whatever you want to do is fine, ultimately 🚲 🚲 🚲 🚲 🚲

🚲

tessr · 2017-05-15T22:28:03Z

database/sinkdb/sinkdb.go

+
+// Exec executes the provided operations. If all of the provided conditionals
+// are met, all of the provided effects are applied atomically.
+func (db *DB) Exec(ctx context.Context, ops ...Op) error {


Does this function get us enough atomicity to do things like migrate multiple tables from pg? (e.g. right now we migrate config and access tokens separately, but that should be atomic)

Oo, I think so. We should be able to pass in all the Set operations and a single sinkdb.IfNotExists("/core/config").

Awesome! Do you want to add a TODO to the migration function?

tessr · 2017-05-15T22:29:25Z

database/sinkdb/sinkdb.go

+
+// AddAllowedMember configures sinkdb to allow the provided address
+// to participate in Raft.
+func (db *DB) AddAllowedMember(ctx context.Context, addr string) error {


Is there a reason that isAllowedMember is on the state and AddAllowedMember is not? Is it just because AddAllowedMember needs to be exported?

Yeah, it needs to be exported and it needs to access raft. The state struct doesn't have access to the *raft.Service struct because the state struct is created and passed in to raft.Start

tessr · 2017-05-15T22:30:15Z

database/sinkdb/state.go


-	return b
+func (s *state) EmptyWrite() (instruction []byte) {


this is great

tessr · 2017-05-15T22:40:26Z

net/http/authz/authorizer.go

-	raftDB      *raft.Service
-	raftPrefix  string
+	sdb         *sinkdb.DB
+	kvPrefix    string


What about keyPrefix?

tessr · 2017-05-15T22:41:50Z

Just a few 🚲 -type comments, but this is starting to look really good!

kr · 2017-05-15T22:51:37Z

net/http/authz/grant.go

 	if err != nil {
 		return nil, errors.Wrap(err)
 	}

-	grants := grantList.Grants
+	grants := grantList.GetGrants()


Nit: I think this can still be just .Grants no? Because grantList is a struct not a pointer.

kr · 2017-05-15T22:57:38Z

database/sinkdb/sinkdb.go

+
+// IfNotExists encodes a conditional to make an instruction
+// successful only if the provided key does not exist.
+func IfNotExists(key string) (op Op) {


Nit: maybe this should not have the final 's', like https://godoc.org/os#IsNotExist, idk

kr · 2017-05-15T23:23:46Z

database/raft/raft.go

 // guaranteed not to read stale data.)
-// This can be slow; for faster but possibly stale reads, see Stale.
-func (sv *Service) Get(ctx context.Context, key string) ([]byte, error) {
+func (sv *Service) RequestRead(ctx context.Context) error {


Nit: if you're happy with this name then cool, but if not then consider WaitRead.

kr · 2017-05-15T23:28:07Z

database/raft/raft.go

-// Set won't have changed the value again, but it is
+// RequestRead requests a linearizable read. Upon successful return,
+// the caller is guaranteed that a read from the Service's state will
+// be linearizable; this is, if a Set happens before a Get, the Get


We can be a lot more concrete with this description now that this method has such a good interface.

"RequestRead requests a linearizable read. Upon successful return, subsequent reads will observe all writes that happened before the call to RequestRead. (There is still …)"

(Or possibly "WaitRead waits for…" if you go with that name.)

kr · 2017-05-15T23:28:53Z

database/raft/raft.go

 	}
 }

-func (sv *Service) get(ctx context.Context, key string) ([]byte, error) {
+func (sv *Service) linearizeRead(ctx context.Context) error {


It could also just be a lowercase version of the public method, so requestRead.

kr · 2017-05-15T23:36:54Z

database/sinkdb/sinkdb.go

+	raft  *raft.Service
+}
+
+type Op struct {


Maybe think about putting this type and its constructors in a separate file op.go. I have a feeling the list will grow in the future.

kr · 2017-05-15T23:41:28Z

database/sinkdb/sinkdb.go

+		return false, err
+	}
+	buf := db.state.get(key)
+	if len(buf) == 0 {


Are we sure this is a safe way to check for "found"? A zero-value protobuf can serialize to nil, which we'll happily insert into the state map (and which will correctly unmarshal back to the zero-value struct). In many cases it prob won't matter, but I think it would be good if the found value returned by this func exactly corresponds to the condition that's tested by IfNotExists.

We were already doing it this way before, so maybe we don't need to change now. But could we at least add a todo?

I think the "right way" would be for get to return a bool and have Get just look at the bool, not the value, before unmarshaling.

yeah, agreed it's wonky. i'll TODO it for now

kr · 2017-05-15T23:53:20Z

core/grants.go

-	if err != nil {
+	var grantList authz.GrantList
+	found, err := a.sdb.Get(ctx, GrantPrefix+x.Policy, &grantList)
+	if err != nil || !found {


When found is false, this will return errors.Wrap(nil), which is nil, which is the right thing to do, but maybe deserves a comment?

kr · 2017-05-15T23:54:54Z

core/grants.go

 		if err != nil {
 			return nil, errors.Wrap(err)
 		}
-		if data == nil {
+		if !ok {


We could also delete this whole if stmt right? (b/c grantList.Grants will be empty)

kr · 2017-05-15T23:55:38Z

core/coreunsafe/reset.go

 	for _, p := range core.Policies {
-		err = rDB.Delete(ctx, core.GrantPrefix+p)
+		err = sdb.Exec(ctx, sinkdb.Delete(core.GrantPrefix+p))


Todo: all these deletes in a big batch.

tessr

This LGTM, but maybe wait for @kr to sign off, too?

kr · 2017-05-16T02:31:58Z

LGTM too.

Split database/raft into two packages, one handling raft-specific logic, the other exposing a consistent kv store interface. database/raft manages the raft.Node and exposes http endpoints for communication between raft nodes. database/sinkdb implements a consistent kv store on top of database/raft. For now, kv data is stored in a simple protocol buffer persisted to the filesystem.

tessr reviewed May 15, 2017

View reviewed changes

jbowens added the PTAL label May 15, 2017

tessr reviewed May 15, 2017

View reviewed changes

database/sinkdb/state.go

return b

func (s *state) EmptyWrite() (instruction []byte) {

Copy link

Contributor

tessr May 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is great

tessr reviewed May 15, 2017

View reviewed changes

kr reviewed May 16, 2017

View reviewed changes

tessr approved these changes May 16, 2017

View reviewed changes

kr approved these changes May 16, 2017

View reviewed changes

jbowens and others added 14 commits May 16, 2017 17:24

s/kvPrefix/keyPrefix

0c201b0

Grants, not GetGrants()

d3fc259

op.go

17153f6

s/RequestRead/WaitRead/

14c9fa2

add batch delete TODO

c955198

remove unnecessary if stmt

5344ada

s/linearizeRead/waitRead/

b38210a

explain errors.Wrap(nil) -> nil

d0d890b

add TODO about found flag

7c6835d

remove redundant return value name

5b8ebe7

s/GetInconsistent/GetStale/

fd1253d

linearizable sp

92b8a00

auto rev id

3dd3b5a

iampogo force-pushed the rethink-kitchen-sink-db branch from f84756f to 3dd3b5a Compare May 16, 2017 17:24

iampogo merged commit 28d3ee5 into main May 16, 2017

iampogo deleted the rethink-kitchen-sink-db branch May 16, 2017 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

database/sinkdb: new package #1192

database/sinkdb: new package #1192

jbowens commented May 15, 2017

tessr May 15, 2017

tessr May 15, 2017

jbowens May 15, 2017

tessr May 15, 2017

tessr May 15, 2017

tessr May 15, 2017

kr May 15, 2017

tessr May 15, 2017

jbowens May 15, 2017 •

edited

Loading

tessr May 15, 2017

tessr May 15, 2017

jbowens May 15, 2017

tessr May 16, 2017

tessr May 15, 2017

jbowens May 15, 2017

tessr May 15, 2017

tessr May 15, 2017

tessr commented May 15, 2017

kr May 15, 2017

kr May 15, 2017

kr May 15, 2017

kr May 15, 2017

kr May 15, 2017

kr May 15, 2017

kr May 15, 2017

jbowens May 16, 2017

kr May 15, 2017

kr May 15, 2017

kr May 15, 2017

tessr left a comment

kr commented May 16, 2017

database/sinkdb: new package #1192

database/sinkdb: new package #1192

Conversation

jbowens commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbowens May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tessr commented May 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tessr left a comment

Choose a reason for hiding this comment

kr commented May 16, 2017

jbowens May 15, 2017 •

edited

Loading