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-3384 Avoid triggering unnecessary resync when working with default collection #6504

Merged
merged 5 commits into from
Oct 13, 2023

Conversation

adamcfraser
Copy link
Collaborator

@adamcfraser adamcfraser commented Oct 7, 2023

CBG-3384

  • Don’t persist syncInfo for empty metadataID (legacy config)
  • Modify requirements for assignment of default metadata ID to a database:
    1. Remove requirement for presence of _sync:seq in default collection
    2. Allow assignment when default collection’s syncInfo is using defaultMetadataID

These avoid triggering resync for databases using only the default collection, in legacy and non-legacy mode.

Also ensures databases share metadataIDs across config groups (CBG-3527)

Integration Tests

Copy link
Collaborator

@torcolvin torcolvin left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -42,8 +42,12 @@ type BootstrapConnection interface {
// TouchMetadataDocument sets the specified property in a bootstrap metadata document for a given bucket and key. Used to
// trigger CAS update on the document, to block any racing updates. Does not retry on CAS failure.
TouchMetadataDocument(ctx context.Context, bucket, key string, property string, value string, cas uint64) (casOut uint64, err error)
// KeyExists checks whether the specified key exists
// KeyExists checks whether the specified key exists in the bucket's default collection
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 say metadata collection here and below for when we migrate this to a separate collection?

Right now default collection = metadata collection but it may not always.

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 intentionally left this comment as default collection because the bootstrap doesn't currently have the concept of a metadata store, and will require some refactoring when we want to add system collection support.

}
return false, err
}
err = getResult.Content(rv)
Copy link
Collaborator

Choose a reason for hiding this comment

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

fix ineffassign

@@ -397,6 +400,9 @@ func InitSyncInfo(ds DataStore, metadataID string) (requiresResync bool, err err

// SetSyncInfo sets syncInfo in a DataStore to the specified metadataID
func SetSyncInfo(ds DataStore, metadataID string) error {
if metadataID == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you drop a comment for why this happens here, for future reference?

return standardMetadataID
}
if exists {
base.WarnfCtx(ctx, "Error checking syncInfo metadataID in default collection - using standard metadataID. Error: %v", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I haven't thoroughly thought this through, but is this going to be called only when there's a cas change, or is this going to log at bootstrap pooling interval?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is only going to be called once per db creation - once the metadataID is assigned (to standardMetadataID in this case), it won't fire again.

- Don’t persist syncInfo for empty metadataID (legacy config)
- Modify requirements for assignment of default metadata ID to a database:
1. Remove requirement for presence of _sync:seq in default collection
2. Allow assignment when default collection’s syncInfo is using defaultMetadataID

These avoid triggering resync for databases using only the default collection, in legacy and non-legacy mode.
If database already exists in registry, new databases (in different config groups) should use the same metadataID.
@adamcfraser adamcfraser assigned torcolvin and unassigned adamcfraser Oct 13, 2023
var legacyConfig rest.LegacyServerConfig
err := base.JSONUnmarshal(legacyConfigBytes, &legacyConfig)
assert.NoError(t, err)
legacyRT.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

move this up to defer at the top of test to protect other tests?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This needs to be closed before opening the non-legacy RTs, so leaving as-is.

@torcolvin torcolvin merged commit 03794e0 into master Oct 13, 2023
26 checks passed
@torcolvin torcolvin deleted the CBG-3384 branch October 13, 2023 21:29
torcolvin pushed a commit that referenced this pull request Oct 31, 2023
…t collection (#6504)

* CBG-3384 Modify computation of metadataID

- Don’t persist syncInfo for empty metadataID (legacy config)
- Modify requirements for assignment of default metadata ID to a database:
1. Remove requirement for presence of _sync:seq in default collection
2. Allow assignment when default collection’s syncInfo is using defaultMetadataID

These avoid triggering resync for databases using only the default collection, in legacy and non-legacy mode.

If database already exists in registry, new databases (in different config groups) should use the same metadataID.
bbrks pushed a commit that referenced this pull request Mar 28, 2024
…t collection (#6504)

* CBG-3384 Modify computation of metadataID

- Don’t persist syncInfo for empty metadataID (legacy config)
- Modify requirements for assignment of default metadata ID to a database:
1. Remove requirement for presence of _sync:seq in default collection
2. Allow assignment when default collection’s syncInfo is using defaultMetadataID

These avoid triggering resync for databases using only the default collection, in legacy and non-legacy mode.

If database already exists in registry, new databases (in different config groups) should use the same metadataID.
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