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-3764 create multi-xattr subdoc APIs #6739

Merged
merged 18 commits into from
Mar 27, 2024
Merged

CBG-3764 create multi-xattr subdoc APIs #6739

merged 18 commits into from
Mar 27, 2024

Conversation

torcolvin
Copy link
Collaborator

@torcolvin torcolvin commented Mar 20, 2024

Update multi xattr subdoc APIs to support multiple xattrs. Where the APIs previously accepted two xattrs (user xattr and sync xattr), continue to allow two xattrs with CBS 7.6 with a CAS check in a loop. If the API previously did not accept two APIs, Couchbase Server 7.2 will return an invalid argument error if multiple xattrs are not specified.

Possible enhancements:

  • enhance rosmar to be able to toggle on BucketStoreFeatureMultiXattrSubdocOperations to be able to test without running two versions of couchbase server
  • Writing a check in all xattr APIs to make sure there are only 1 or 2 supported xattrs if using CBS < 7.6 which provides a bit easier message than one.
  • Write a check in all xattr APIs to make sure that xattrKeys are never empty string. This was a late fix in integration tests from a somewhat opaque gocb error message. Alternatively, error in rosmar if any xattrKeys are empty.
  • Move ErrXattrNotFound and ErrXattrPartialFound to sg-bucket and return in rosmar?

Things to look for in the PR:

  • Did I remove any operations that should be retried to work a single time?
  • Should I rename any functions to make it more clear which is the function that will do a cas based retry and which does the actual operation?
  • Double checking any cases where I change logging for redaction OR error wrapping.

couchbaselabs/rosmar#25
couchbase/sg-bucket#112

Pre-review checklist

  • Removed debug logging (fmt.Print, log.Print, ...)
  • Logging sensitive data? Make sure it's tagged (e.g. base.UD(docID), base.MD(dbName))
  • Updated relevant information in the API specifications (such as endpoint descriptions, schemas, ...) in docs/api

Dependencies (if applicable)

  • Link upstream PRs
  • Update Go module dependencies when merged

Integration Tests

Copy link
Collaborator

@adamcfraser adamcfraser left a comment

Choose a reason for hiding this comment

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

A few minor comments/questions but otherwise looks good to me.

base/collection_xattr.go Outdated Show resolved Hide resolved
db/database.go Outdated
Comment on lines 1829 to 1832
xattrKeys := []string{base.SyncXattrName}
if db.userXattrKey() != "" {
xattrKeys = append(xattrKeys, db.userXattrKey())
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

A helper function to build the xattr keys wouldn't be a bad idea, since we're going to need to enhance it to add _vv in the near future.

base/collection_xattr.go Show resolved Hide resolved
base/collection_xattr.go Outdated Show resolved Hide resolved
@adamcfraser adamcfraser assigned torcolvin and unassigned adamcfraser Mar 25, 2024
- change comment to refer to feature and not CBS version
- remove defensive check since gocb does a good job with error message
- create syncAndUserXattrKeys helper function
@torcolvin torcolvin assigned adamcfraser and unassigned torcolvin Mar 26, 2024
adamcfraser
adamcfraser previously approved these changes Mar 26, 2024
@adamcfraser
Copy link
Collaborator

LGTM - ready to merge once dependencies are merged and go.mod updated.

@torcolvin torcolvin merged commit 8b976f5 into master Mar 27, 2024
30 of 31 checks passed
@torcolvin torcolvin deleted the CBG-3764 branch March 27, 2024 00:04
bbrks pushed a commit that referenced this pull request Mar 28, 2024
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