Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CBG-3849 Remove inline cas handling for xattr config persistence #6850

Merged
merged 3 commits into from
May 30, 2024

Conversation

adamcfraser
Copy link
Collaborator

@adamcfraser adamcfraser commented May 30, 2024

CBG-3849

Remove inline cas usage for xattr config persistence - use standard document CAS semantics.

Integration Tests

Ran additional integration test with forced use of XattrBootstrapPersistence for all tests:
https://jenkins.sgwdev.com/job/SyncGateway-Integration/2463/
Triaged the three failures from that run:

  • base.TestXattrConfigPersistence
  • rest.TestPersistentConfigNoBucketField
  • rest.TestPersistentDbConfigWithInvalidUpsert
    • These two tests rely on modifying the config manually in the bucket, and so aren't compatible with XattrBootstrapPersistence. Non-trivial to modify or skip these tests, so leaving as-is.

@adamcfraser
Copy link
Collaborator Author

CI failure on test-race appears to be unrelated issue.

if bodyErr != nil {
restoreErr := xbp.restoreDocumentBody(c, key, valuePtr, strCas)
var restoreErr error
casOut, restoreErr = xbp.restoreDocumentBody(c, key, valuePtr, res.Cas())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there's an error here, I think casOut will be updated to 0, which might not be correct? The tombstone document will have a cas.

OTOH, the cas isn't super useful

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also see how if there's no document, then it should have a zero cas. I'm not sure what to do here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're correct - if the restore fails we should return the cas of the current version of the document (the tombstone), since we're returning the tombstone's xattr.

// Restore a deleted document's body. Rewrites metadata, but preserves previous cfgCas
func (xbp *XattrBootstrapPersistence) restoreDocumentBody(c *gocb.Collection, key string, value interface{}, cfgCas string) error {
// Restore a deleted document's body. Rewrites metadata
func (xbp *XattrBootstrapPersistence) restoreDocumentBody(c *gocb.Collection, key string, value interface{}, cas gocb.Cas) (casOut gocb.Cas, err error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We aren't using a cas check here because there aren't cas checks on insert. I think you can remove cas from an argument here.

We are sure that there is no document existing because on gocb.StoreSemanticsInsert

@torcolvin torcolvin assigned adamcfraser and unassigned torcolvin May 30, 2024
@adamcfraser adamcfraser assigned torcolvin and unassigned adamcfraser May 30, 2024
@torcolvin torcolvin enabled auto-merge (squash) May 30, 2024 22:56
@torcolvin torcolvin merged commit 7492635 into main May 30, 2024
33 of 34 checks passed
@torcolvin torcolvin deleted the CBG-3849 branch May 30, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants