chore: add fdv2 store & update sources to use fdv2 protocol definitions #192
Conversation
cwaldren-ld
force-pushed
the
cw/sdk-557/fdv2-store
branch
from
485c597
to
941fee4
Compare
cwaldren-ld
force-pushed
the
cw/sdk-557/fdv2-store
branch
from
c37485e
to
4d03305
Compare
| @@ -16,7 +16,7 @@ var ( | |||
| deleteDataRequiredProperties = []string{"path", "version"} //nolint:gochecknoglobals | |||
There was a problem hiding this comment.
This is just a reversion of recent changes that made these public (for use in datasourcev2), but are no longer necessary.
cwaldren-ld
force-pushed
the
cw/sdk-557/fdv2-store
branch
2 times, most recently
from
9d79eaf
to
c91fd1e
Compare
cwaldren-ld
force-pushed
the
cw/sdk-557/fdv2-store
branch
from
c91fd1e
to
024c0df
Compare
| return s.active | ||
| } | ||
|
|
||
| //nolint:revive // Implementation for ReadOnlyStore. |
There was a problem hiding this comment.
Can we return a pointer to the right store once for each evaluation?
|
@keelerm84 , realized why I was confused - the data system config PR has some of the other improvements in the streaming/polling sources (like replacing strings with constants). I tried to only touch the sources enough to get them to interact with the new store. I'll have another PR that focuses on cleaning up the sources. |
cwaldren-ld
force-pushed
the
cw/sdk-557/fdv2-store
branch
from
38fb529
to
f72ebc0
Compare
cwaldren-ld
changed the title
| item, err := kind.DeserializeFromJSONReader(&r) | ||
| return item, err |
There was a problem hiding this comment.
I believe this is equivalent:
| item, err := kind.DeserializeFromJSONReader(&r) | |
| return item, err | |
| return kind.DeserializeFromJSONReader(&r) |
Which makes parseItem a single line method. At that point, is there value in having it as a a separate function?
|
|
||
| parseItem := func(r jreader.Reader, kind datakinds.DataKindInternal) (ldstoretypes.ItemDescriptor, error) { | ||
| item, err := kind.DeserializeFromJSONReader(&r) | ||
| return item, err | ||
| } | ||
|
|
||
| for _, event := range events { | ||
| switch event.Event() { | ||
| case putEventName: | ||
| switch fdv2proto.EventName(event.Event()) { |
There was a problem hiding this comment.
I can't comment on the actual line, but parseItem here suffers from the same issue I pointed out above.
There was a problem hiding this comment.
It turns out this whole bit of logic (the event parsing) doesn't work properly due to the streaming JSON parser. I'm going to have to refactor this to more closely match how the existing datasource (V1) parsers work - which is two passes.
The reason is because we might visit the "object" field before we've visited the kind field.
| func ToStorableItems(events []Event) []ldstoretypes.Collection { | ||
| flagCollection := ldstoretypes.Collection{ | ||
| Kind: datakinds.Features, | ||
| Items: make([]ldstoretypes.KeyedItemDescriptor, 0), |
There was a problem hiding this comment.
Do you think it's worth reserving some capacity here to prevent the additional allocations?
There was a problem hiding this comment.
We don't know the size up front, although we could calculate it. I think I'd rather let Go's allocation strategy do its thing and if it's a problem we can fix it.
This fully converts the datasources to use `fdv2` protocol domain objects.
Use the new `toposort` package to insert items int persistent store in dependency order.
cwaldren-ld
force-pushed
the
cw/sdk-557/fdv2-store
branch
from
4681961
to
e5bb240
Compare
cwaldren-ld
force-pushed
the
cw/sdk-557/fdv2-store
branch
from
f6fe6c0
to
e6ddefe
Compare
This adds the FDv2 dual-mode store, which supports serving data requests from an in-memory store or from a persistent store (with cache).