Feature/issue 3738 norev #3763
Feature/issue 3738 norev #3763
Conversation
|
Hey @tleyden , if you rebase this on the latest |
This reverts commit 9e54548.
tleyden
force-pushed
the
feature/issue_3738_norev
branch
from
80ec81f
to
6d9e29c
Compare
rest/blip_sync.go
Outdated
|
|
||
| func (bh *blipHandler) sendNoRev(err error, sender *blip.Sender, seq db.SequenceID, docID string, revID string, knownRevs map[string]bool, maxHistory int) { | ||
|
|
||
| bh.Logf(base.LevelDebug, base.KeySync, "Sending norev %q %s based on %d known. User:%s", base.UD(docID), revID, len(knownRevs), base.UD(bh.effectiveUsername)) |
There was a problem hiding this comment.
I think any logging for the norev message should include the error details.
rest/blip_sync.go
Outdated
| } | ||
| } | ||
|
|
||
| func (bh *blipHandler) sendNoRev(err error, sender *blip.Sender, seq db.SequenceID, docID string, revID string, knownRevs map[string]bool, maxHistory int) { |
There was a problem hiding this comment.
knownRevs and maxHistory aren't relevant for sendNoRev, can be removed.
| outrq.Properties["sequence"] = string(seqJSON) | ||
| if marshalErr == nil { | ||
| outrq.Properties["sequence"] = string(seqJSON) | ||
| } |
There was a problem hiding this comment.
Looking at the spec, sequence is generally optional in a rev message, other than the case where a changes message hasn't been sent. I don't think we have that scenario for rev messages sent by Sync Gateway. If we can omit sequence altogether, we should file a ticket for that change for Iridium.
There was a problem hiding this comment.
What problem gets solved by omitting the sequence whenever possible? (performance?)
It seems potentially useful from a debugging / diagnostics point of view, but the number of cases where it's useful might be narrow: for example if there was a bug that caused peer sync gateways to send back different versions of a particular revision of a doc, it would be useful to see what sequence they thought the revisions were based off of. (I realize this might not be a realistic scenario, I'm just saying that there might be scenarios we can't think of that could be useful to have more info)
Since a norev message is always solicited, I will update the spec to say (optional), without the unsolicited caveat. Along those lines, it could always be omitted from norev messages, but I wanted to understand the motivation.
If we can omit sequence altogether, we should file a ticket for that change for Iridium.
I'll file that and we can move the discussion over there. I think for this PR we could just keep things as-is, and as part of that Iridium ticket go back and change the behavior for both rev and norev messages.
There was a problem hiding this comment.
Motivation is to reduce CPU cost on SG (marshalling sequence) and transport cost to clients. Agree that this doesn't need to be changed for this PR.
If there are actually low-level troubleshooting benefits, we should address those via logging. (although I don't know of any cases where id/rev could be associated with different sequences).
There was a problem hiding this comment.
Looking closer, the sequence that we're sending in the rev message is always just the sequence we previously sent in the changes message - we're not attempting to recalculate it from the document's current metadata. Given that, I think there's even less value in sending it back to the client on the rev message. I'm not sure why the changes response from CBL is specifying sequence, in fact. Should be reviewed w/ #3764
|
The jenkins job finished without issues, so I'm going to go ahead and merge |
* Repro attempt for SG #3738 * /_flush_rev_cache endpoint for easy manual testing * TODOs on fix * Clean up test * First pass at implmenting norev * Update test helper to deal with norev messages * Translate error to status code * Avoid panic if there is a marshal err for the sequence * Revert "/_flush_rev_cache endpoint for easy manual testing" This reverts commit 9e54548. * Update test comment * PR feedback
Fixes #3738