CBG-4027 Treat on-demand import for GET errors as not found #6957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ensures that these are correctly handled as norev messages when encountered during replication.
adamcfraser
force-pushed
the
CBG-4027-update
branch
from
df5ae30
to
f806d9d
Compare
torcolvin
previously approved these changes
Jul 12, 2024
db/crud.go
Outdated
| } else if importErr != nil { | ||
| return nil, importErr | ||
| // Treat any other failure to perform an on-demand import as not found | ||
| base.DebugfCtx(ctx, base.KeyImport, "Unable to import doc %q during on demand import for get - will be treated as not found. Reason: %v", base.UD(docid), err) |
There was a problem hiding this comment.
If there's an error that would be 503, like gocb.OverloadError, do we want to skip this doc to not be found again?
Or is the assumption that this would be caught by the traditional import feed and then updated after?
Line 142 in c6b3b3c
or
https://github.com/couchbase/sync_gateway/blob/main/db/crud.go#L1938 should be able to trigger this behavior if there's a leaky datastore that can return 503s?
There was a problem hiding this comment.
That's correct, if there's a failure to write the doc for an on-demand import, this will fall back to the feed-based import. In this case clients who had requested the previous rev will get a norev, and will get the current rev when it is imported.
bbrks
previously requested changes
Jul 12, 2024
db/crud.go
Outdated
| } else if importErr != nil { | ||
| return nil, importErr | ||
| // Treat any other failure to perform an on-demand import as not found | ||
| base.DebugfCtx(ctx, base.KeyImport, "Unable to import doc %q during on demand import for get - will be treated as not found. Reason: %v", base.UD(docid), err) |
There was a problem hiding this comment.
Suggested change
| base.DebugfCtx(ctx, base.KeyImport, "Unable to import doc %q during on demand import for get - will be treated as not found. Reason: %v", base.UD(docid), err) | |
| base.DebugfCtx(ctx, base.KeyImport, "Unable to import doc %q during on demand import for get - will be treated as not found. Reason: %v", base.UD(docid), importErr) |
gregns1
approved these changes
Jul 15, 2024
adamcfraser
dismissed
bbrks’s stale review
Requested change has been made, reviewed/approved by Greg.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
CBG-4027
Ensures that these are correctly handled as norev messages when encountered during replication, as they always represent a case where the requested revision was not available.
Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2572/