fix: Incremental tables should not delete stale #594
Conversation
|
The new test is the first time instance of memdb being used by multiple tables at once, and it turned out we had a data race there. I fixed this by adding a lock around the memdb internal map. |
⏱️ Benchmark resultsComparing with 506d4cc
|
erezrokah
left a comment
erezrokah
left a comment
There was a problem hiding this comment.
Looks good, added one comment for clarification
| if err := p.DeleteStale(ctx, tables, sourceName, syncTime); err != nil { | ||
| include := func(t *schema.Table) bool { return true } | ||
| exclude := func(t *schema.Table) bool { return t.IsIncremental } | ||
| nonIncrementalTables, err := tables.FilterDfsFunc(include, exclude) |
There was a problem hiding this comment.
Are tables already filtered by the glob patterns in the spec?
Making sure we don't return all tables here since we include everything.
Also since include is always true, maybe add only the exclude part for now?
There was a problem hiding this comment.
Right, tables are already filtered by glob patterns at this point (previously we were passing all these tables in to delete stale, now we're filtering out the incremental ones from that list first).
FilterDfsFunc is now also used by FilterDfs internally, and that needs both include and exclude.
🤖 I have created a release *beep* *boop* --- ## [1.24.2](v1.24.1...v1.24.2) (2023-01-11) ### Bug Fixes * Incremental tables should not delete stale ([#594](#594)) ([d45e230](d45e230)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
When
overwrite-delete-stalewrite mode is used, the delete stale should exclude incremental tables. We still wantdelete-stalebehavior for non-incremental tables, but incremental tables have the expectation that previously synced rows should remain there.This also fixes a small bug where we were deleting stale entries twice for managed writers, which was caught by the new test.