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-3291 Define macro expansion in MutateInOpts #6419

Merged
merged 7 commits into from
Sep 21, 2023
Merged

Conversation

adamcfraser
Copy link
Collaborator

@adamcfraser adamcfraser commented Sep 15, 2023

CBG-3291

Uptake sg-bucket changes to pass macro expansions for a given operation to the XattrStore implementation (*Collection), instead of hardcoding in the low-level subdoc operations.

The macro expansion options are set in three places for non-test code:

  • updateAndReturnDoc, handles standard writes/imports via WriteUpdateWithXattr
  • migrateMetadata, performs migration via WriteWithXattr
  • resyncDocument, uses WriteUpdateWithXattr (different retry than updateAndReturnDoc)

Prerequisite for the expanded use of macro expansion for HLV properties.

Also includes some cleanup of the XattrStore interface to remove internal-only operations.

Dependencies (if applicable)

Integration Tests


// initializeMutateInSpec will append macro expansions defined in MutateInOptions to the provided
// gocb MutateInSpec.
func (c *Collection) appendMacroExpansions(mutateInSpec []gocb.MutateInSpec, opts *sgbucket.MutateInOptions) []gocb.MutateInSpec {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm loosely inclined to make this not be a method on Collection because it doesn't have a dependency on Collection

@@ -2582,3 +2585,25 @@ func (db *DatabaseCollectionWithUser) CheckProposedRev(ctx context.Context, doci
return ProposedRev_Conflict, doc.CurrentRev
}
}

const (
xattrMacroCas = "cas"
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we pull this value from sg-bucket? not convinced either way

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 it belongs in SGW as this defines the document property that's using the macro expansion, not the macro expansion type.

@@ -1899,6 +1899,10 @@ func (db *DatabaseCollectionWithUser) updateAndReturnDoc(ctx context.Context, do
if db.UseXattrs() || upgradeInProgress {
var casOut uint64
// Update the document, storing metadata in extended attribute
if opts == nil {
opts = &sgbucket.MutateInOptions{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this DefaultMutateInOpts() ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's possible that the caller has already set MutateInOptions.PreserveExpiry, so just initializing here and then adding the MacroExpansion (for either case) below.

Comment on lines +1829 to +1831
opts := &sgbucket.MutateInOptions{
MacroExpansion: macroExpandSpec(base.SyncXattrName),
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this DefaultMutateInOpts() ?

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 intended DefaultMutateInOpts() to be test-only, as I thought the non-test usages should be more intentional about what they are setting. I think I'd prefer to leave as-is for now, and possibly revisit during future HLV work that makes the default more complex.

Uptake sg-bucket changes to pass macro expansions for a given operation to the XattrStore implementation (*Collection), instead of hardcoding in the low-level subdoc operations.

The macro expansion options are set in three places for non-test code:
- updateAndReturnDoc, handles standard writes/imports via WriteUpdateWithXattr
- migrateMetadata, performs migration via WriteWithXattr
- resyncDocument, uses WriteUpdateWithXattr (different retry than updateAndReturnDoc)

Prerequisite for the expanded use of macro expansion for HLV properties.

Also includes some cleanup of the XattrStore interface to remove internal-only operations.
torcolvin
torcolvin previously approved these changes Sep 20, 2023
@adamcfraser adamcfraser enabled auto-merge (squash) September 21, 2023 16:49
@adamcfraser adamcfraser merged commit 25fc3c5 into master Sep 21, 2023
@adamcfraser adamcfraser deleted the CBG-3291-rebase branch September 21, 2023 16:59
@bbrks bbrks mentioned this pull request Sep 22, 2023
1 task
bbrks pushed a commit that referenced this pull request Mar 28, 2024
* Rebase on master

* go.mod bump for merged dependencies

* CBG-3291 Define macro expansion in MutateInOpts

Uptake sg-bucket changes to pass macro expansions for a given operation to the XattrStore implementation (*Collection), instead of hardcoding in the low-level subdoc operations.

The macro expansion options are set in three places for non-test code:
- updateAndReturnDoc, handles standard writes/imports via WriteUpdateWithXattr
- migrateMetadata, performs migration via WriteWithXattr
- resyncDocument, uses WriteUpdateWithXattr (different retry than updateAndReturnDoc)

Prerequisite for the expanded use of macro expansion for HLV properties.

Also includes some cleanup of the XattrStore interface to remove internal-only operations.

* Integration test fixes

* Fix attachment test

* PR suggestions/fixes

* Update dependencies
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