New memdb-based state store #1291
Conversation
| &reply.QueryMeta, | ||
| state.QueryTables("ServiceNodes"), | ||
| state_store.NewMultiWatch( |
There was a problem hiding this comment.
Not sure how you guys feel about this where we expose the table names up at this level. I really didn't like how KV was special so I normalized that, but I do see that this exposes lower-level details about the tables up. It would be easy to provide a string-based lookup thing like the old interface where you pass the name of the function you are calling here.
There was a problem hiding this comment.
I'm actually going to push this down and keep something similar to the old interface (except for KV). This will be a better abstraction and will keep us from needing the state import in several places. Should be an easy change.
slackpad
force-pushed
the
f-memdb
branch
2 times, most recently
from
b23a1b9 to
bfd65e0
Compare
| if err := encoder.Encode(raw); err != nil { | ||
| return err | ||
| } | ||
| entries, err := s.state.KVSDump() |
There was a problem hiding this comment.
Could we just expose the iterator here?
| Name: "session", | ||
| AllowMissing: true, | ||
| Unique: false, | ||
| Indexer: &memdb.StringFieldIndex{ |
There was a problem hiding this comment.
I think we can use the UUIDFieldIndex here
There was a problem hiding this comment.
Got this - added a UUID maker just for unit tests.
|
Major feedback:
Otherwise LGTM |
…drift bug. Realized that the conversions ServiceNode <-> NodeService were incomplete in a few places so centralized those and added some tests.
|
Restore optimization is the last change based on Armon's feedback - working on that now. |
|
@armon and @ryanuber all the outstanding comments have been addressed so please take another look. I'm going to leave the TODO in |
| state := s.fsm.State() | ||
| _, node, err := state.GetNode(member.Name) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to lookup node %s: %s", member.Name, err) |
There was a problem hiding this comment.
It seems weird that this is the only error we prefix. Ideally they would all have some context around them. Does the returned error already contain the context for the nested calls below?
There was a problem hiding this comment.
Yeah these will end up pretty worthless, actually. I'll delete the prefix and just return err for these.
|
@slackpad this all looks super solid! We should also remove the entry in the FAQ that mentions Consul's use of LMDB. I wasn't able to get a diff of the changes on the state store from where I left off and you picked up (because they are so large), but I perused the file and things looked sane. 👍 |
|
Party! 🎉 |
The major integration is done so I thought I'd open this up to start getting eyeballs on it.
Here's my current list of things outstanding:
go vetMost of this is adding some small unit tests, but the lindex item could touch a few things.