Backfill indexing #1290
Backfill indexing #1290
Conversation
internal/statestore/backfill.go
Outdated
| } | ||
| defer handleConnectionClose(&redisConn) | ||
|
|
||
| // TODO: review usage of version or stick with Generation |
There was a problem hiding this comment.
Here (and other todo) can you add a task on your backlog for this? I think generation is going to work here, so I'm worried nobody is going to look at this code and the todo will stick around...
There was a problem hiding this comment.
Otherwise, create an issue on github, saying these todos need to be cleaned up, and reference the issue in the todo, eg
// TODO(#1234): clean the gutters
There was a problem hiding this comment.
Yes, absolutely! I think there's a conversation needed to happen so about the Version/Generation issue, and as soon that clears, i'll delete the TODOs i've put
There was a problem hiding this comment.
#1294 Was created, thanks Hugo
There was a problem hiding this comment.
Remove the todo, and this can be merged.
There was a problem hiding this comment.
Go for merging @Laremere 👍
| // deindex one backfill, one backfill exist | ||
| err := service.DeindexBackfill(ctx, backfills[0].Id) | ||
| require.Nil(t, err) | ||
| verifyBackfills(service, backfills[1:2]) |
There was a problem hiding this comment.
Could you please add one more test with updating Generation to see, if HSET and IndexBackfill would work as expected - BackfillID would be mapped to 1000 now.
newGen : = 1000
backfills[1].Generation = newGen
bf, service.UpdateBackfill(ctx, backfills[1], []string{}))
require.NoError(t, service.IndexBackfill(ctx, backfills[1]))
There was a problem hiding this comment.
I don't see exactly what you want tested here. If you are concerned about HSET the documentation says if the key already exists, it is overwritten with the new value. And the scenario you are proposing i have it tested in the backfill caching update tests
Redis documentation for HSET says:
If key does not exist, a new key holding a hash is created. If field already exists in the hash, it is overwritten.
There was a problem hiding this comment.
Yes, just wanted to test update the of the index, just to check it is updated
Co-authored-by: Alexander Apalikov <[email protected]>
What this PR does / Why we need it:
This PR adds
IndexBackfill,DeindexBackfillandGetIndexedBackfillsfuncs to the statestore, funcs used inQueryServicebut, for code sanity, it was decided to put them in a separate PRWhich issue(s) this PR fixes:
This is related to #1280 which is also related to #1240