CBG-4270: Implement sequence-based storage for BlipTesterClient #7226
Conversation
There was a problem hiding this comment.
I only looked through part of this and mostly not the changes in blip_client_test.go but it would be helpful to me to have an overall design strategy possibly even in a comment for how document bodies and revisions are stored on the body for a pull replication and push replication.
It might make sense for me to meet and have you walk me through the design strategy a bit to make the review faster for me to do.
…ed - handle broadcast wake on close
… rely on the context chaining
There was a problem hiding this comment.
I think the code generally looks fine. I have some readability improvements I've made to avoid so many ctx.Err() != nil checks which I think were made defensively, and split up some of the complexity in docsSince. As discussed offline, this code might not live forever anyway and get replaced by active replicator code.
I did spend a considerable amount of time debugging why some of the blip delta sync tests fail on my local machine, only when running multiple tests at once. I never got to the bottom of why they failed and I am not sure that they were totally successfully passing before either. Running these on a different system leads to passing. That said, the delta sync tests are not run through github actions, so they might fail.
I will push up a branch (on top of this one) where you can look at the changes I've made to help debug, and decide to keep or remove them.
- remove AssertfCtx / err requirements so that there is a better traceback in the case of failure. In the case of
PushUnsolicitedRevwe need more information from the error message, but generally we just want a blip message to fail the test harness, preferably without passing the error back up. This results in a lot of code changes. - removes most of the logging
- removes channel close from docSince since this is unused
- remove very confusing
UnsubPushChangesfunction. - Added some docstrings to try to understand the code better.
I am fine if you don't want this branch.
I think it is worth considering whether some of the assertion handling to push into testify require assertions with tracebacks will make it easier to debug the code on anemone, and if we want to do a painful merge once. Maybe evaluate how the merge/rebase of this PR will be onto anemone and whether significant changes were made there.
rest/blip_client_test.go
Outdated
| if ctx.Err() != nil { | ||
| close(ch) | ||
| return | ||
| } | ||
| c.TB().Logf("docsSince: sinceVal=%d", sinceVal) | ||
| for _, doc := range c.OneShotDocsSince(ctx, sinceVal) { | ||
| select { | ||
| case <-ctx.Done(): | ||
| close(ch) | ||
| return | ||
| case ch <- doc: | ||
| c.TB().Logf("sent doc %q to changes feed", doc.id) | ||
| sinceVal = doc.latestSeq() | ||
| } | ||
| } | ||
| if !continuous { | ||
| c.TB().Logf("opts.Continuous=false, breaking changes loop") | ||
| break | ||
| } |
There was a problem hiding this comment.
readability nit here:
| if ctx.Err() != nil { | |
| close(ch) | |
| return | |
| } | |
| c.TB().Logf("docsSince: sinceVal=%d", sinceVal) | |
| for _, doc := range c.OneShotDocsSince(ctx, sinceVal) { | |
| select { | |
| case <-ctx.Done(): | |
| close(ch) | |
| return | |
| case ch <- doc: | |
| c.TB().Logf("sent doc %q to changes feed", doc.id) | |
| sinceVal = doc.latestSeq() | |
| } | |
| } | |
| if !continuous { | |
| c.TB().Logf("opts.Continuous=false, breaking changes loop") | |
| break | |
| } | |
| defer close(ch) | |
| for _, doc := range c.OneShotDocsSince(ctx, sinceVal) { | |
| select { | |
| case <-ctx.Done(): | |
| return | |
| case ch <- doc: | |
| sinceVal = doc.latestSeq() | |
| } | |
| } | |
| if !continuous { | |
| break | |
| } |
Without this change, I don't understand how the loop closes in the non continuous case.
There was a problem hiding this comment.
It closes from the context when that is cancelled, but otherwise doesn't.
The loop will sit waiting in the OneShotDocsSince iterator until a new sequence is available on the client via the c._seqCond.Wait()
CBG-4270
BlipTesterClient now maintains a client-side sequence number, associated with every document update made on the client. This is independent of the Sync Gateway sequence.
Document storage is sequence-based, and we store all versions of documents for the purposes of history and test assertions. No cleanup is made on these old versions, since it's not expected that we'll be writing extremely long-running tests using this.
We build a client-side "changes feed" by iterating over all sequences and emitting the latest version of each document. There is no client-side checkpointing, but tests can start changes feeds from a specific sequence when required. Most tests can just use a continous changes feed running in the background.
The client's changes feed produces
ProposeChangesin batches of only1item at a time, rather than a more real-world example of20- since the code to produce batches and a flush timeout of changes is not implemented. Filed CBG-4401 as follow-up.TODO
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2844/GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2855/GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2863/