Stream the full set of predicates and types during a snapshot. #5444
Conversation
Otherwise, updates to the schema would not be picked up.
github-actions
bot
added
the
area/schema
|
Still working on modifying the existing snapshot test to verify this but @danielmai has already manually tested the change. |
|
Test fails sometimes with this error in badger: alpha2 | panic: close of closed channel @jarifibrahim can you take a look? I'll push the test soon. It's in the worker package which has a custom docker-compose.yml, FYI. |
|
@jarifibrahim Actually, the failure is consistent. Daniel didn't see this but I suspect it might be because in my test the predicate I am trying to drop doesn't have any data. It should still work though but SignalAndWait is trying to close the channel more than once. |
There was a problem hiding this comment.
Reviewed 3 of 4 files at r1, 3 of 5 files at r4, 2 of 2 files at r6.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @martinmr and @vvbalaji-dgraph)
worker/snapshot.go, line 118 at r6 (raw file):
} func cleanupSchema(ctx context.Context, kvs *pb.KVS) error {
deleteStalePreds
worker/snapshot.go, line 144 at r6 (raw file):
"Cannot delete removed predicate %s after streaming snapshot: %v", pred, err) errors.Wrapf(err, "cannot delete removed predicate %s after streaming snapshot",
this looks wrong. errors.Wrapf isn't being used by anyone. Should this be returned?
worker/snapshot.go, line 161 at r6 (raw file):
if _, ok := snapshotTypes[typ]; !ok { if err := schema.State().DeleteType(typ); err != nil { errors.Wrapf(err, "cannot delete removed type %s after streaming snapshot", typ)
you want to return these?
worker/snapshot.go, line 213 at r6 (raw file):
} return item.Version() >= snap.SinceTs
This is cheap. I'll put that up.
if item.Version() >= snap.SinceTs { return true }
x.Parse logic here.
Technically, you could further optimize this to use the fact that schema is stored at TS=1. So, you could say: if item.Version() == 1, then do x.Parse.
There was a problem hiding this comment.
Reviewable status: 5 of 8 files reviewed, 4 unresolved discussions (waiting on @manishrjain and @vvbalaji-dgraph)
worker/snapshot.go, line 118 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
deleteStalePreds
Done.
worker/snapshot.go, line 144 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
this looks wrong. errors.Wrapf isn't being used by anyone. Should this be returned?
Done. Yes, it should be returned.
worker/snapshot.go, line 161 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
you want to return these?
Done.
worker/snapshot.go, line 213 at r6 (raw file):
Previously, manishrjain (Manish R Jain) wrote…
This is cheap. I'll put that up.
if item.Version() >= snap.SinceTs { return true }
x.Parse logic here.
Technically, you could further optimize this to use the fact that schema is stored at TS=1. So, you could say: if item.Version() == 1, then do x.Parse.
Done.
Type and predicate keys always have a timestamp of 1 so they are not being picked up by every snapshot. This PR adds logic to send the entire schema in the snapshot and to delete predicates and types not present in the snapshot (since they were deleted in between snapshots).
Type and predicate keys always have a timestamp of 1 so they are not being picked up by every snapshot. This PR adds logic to send the entire schema in the snapshot and to delete predicates and types not present in the snapshot (since they were deleted in between snapshots).
…h-io#5444) Type and predicate keys always have a timestamp of 1 so they are not being picked up by every snapshot. This PR adds logic to send the entire schema in the snapshot and to delete predicates and types not present in the snapshot (since they were deleted in between snapshots).
Internal ref:
Type and predicate keys always have a timestamp of 1 so they are not being picked up by every snapshot.
This PR adds logic to send the entire schema in the snapshot and to delete predicates and types
not present in the snapshot (since they were deleted in between snapshots).
This change is