CBG-3157: Add opt-in send replacementRevs feature when encountering norev #6822
Conversation
bbrks
changed the title
db/blip_sync_context.go
Outdated
| @@ -579,7 +579,7 @@ func (bsc *BlipSyncContext) sendBLIPMessage(sender *blip.Sender, msg *blip.Messa | |||
| } | |||
|
|
|||
| func (bsc *BlipSyncContext) sendNoRev(sender *blip.Sender, docID, revID string, collectionIdx *int, seq SequenceID, err error) error { | |||
| base.DebugfCtx(bsc.loggingCtx, base.KeySync, "Sending norev %q %s due to unavailable revision: %v", base.UD(docID), revID, err) | |||
| base.TracefCtx(bsc.loggingCtx, base.KeySync, "Sending norev %q %s due to unavailable revision: %v", base.UD(docID), revID, err) | |||
There was a problem hiding this comment.
Do we already log this information elsewhere, which is why we want to lower to Trace here? I thought norevs were generally unexpected and merited the debug logging.
There was a problem hiding this comment.
Good catch. I had intended to move the debug logging up a level so we could get more contextual information about why we're sending the norev.
Have covered all sendNoRev cases now with appropriate log messages.
| base.DebugfCtx(bsc.loggingCtx, base.KeySync, "Unavailable revision for %q %s - finding replacement: %v", base.UD(docID), revID, originalErr) | ||
|
|
||
| // try the active rev instead as a replacement | ||
| replacementRev, replacementRevErr := handleChangesResponseCollection.GetRev(bsc.loggingCtx, docID, "", true, nil) |
There was a problem hiding this comment.
There's some inefficiency here, as we would have fetched the current version of the doc from the bucket when the revCacheLoader attempted to fetch the original (not found) rev.
Since the frequency of replacementRev is expected to be low I think we can live with this, but could consider filing a backlog enhancement to enhance revCacheLoader to have the ability to return activeRev during the initial fetch (or at least stick it in the rev cache).
There was a problem hiding this comment.
I'm not sure it's worth the complexity to cover this one case, and this is the only case we'd need that behaviour.
Are there stats we should add now (SendNoRevCount and SendReplacementRevCount?) to determine the frequency of this codepath to justify doing this work in future?
There was a problem hiding this comment.
The stats do seem like they might be generally useful - both for understanding frequency and understanding what's going on in an environment. I'll leave it to you whether to include in this PR or file a followup ticket.
There was a problem hiding this comment.
Will do as an immediate follow-up/PR (CBG-3960)
…he BLIP messages for test assertions from a doc+rev pair
CBG-3157
Add opt-in replacement revs feature.
GetRevIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2460/