-
Notifications
You must be signed in to change notification settings - Fork 0
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-3703 create a basic bucket to bucket XDCR implementation [HAS DEPENDENCY] #29
Conversation
- uses new common elements from sg-bucket - absorbs DataStoreName and GetCollectionID into DataStore to avoid duplication - The implementation only copies documents with xattrs, and excludes specially named documents. It does not implement _vv, _mou, or _sync handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good I think! Just couple of Q's in comments. Will let Adam have final say too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few questions/clarifications.
xdcr.go
Outdated
fromBucket: fromBucket, | ||
toBucket: toBucket, | ||
replicationID: fmt.Sprintf("%s-%s", fromBucket.GetName(), toBucket.GetName()), | ||
fromBucketCollectionIDs: map[uint32]sgbucket.DataStoreName{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the expected handling if there are collections defined in fromBucket that aren't present in toBucket? Should we be checking for this now instead of logging a warning for every mutation that arrives for a non-existent collection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see below that we're only supporting the default collection at this point? (we're not passing any scopes args to the DCP feed). Are you thinking of adding that later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was an oversight and I've added tests for the non default collection.
xdcr.go
Outdated
|
||
// getFromBucketCollectionName returns the collection name for a given collection ID in the from bucket. | ||
func (r *XDCR) getFromBucketCollectionName(collectionID uint32) (sgbucket.DataStoreName, error) { | ||
dsName, ok := r.fromBucketCollectionIDs[collectionID] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we need to know the set of fromBucket collections when we start the DCP feed anyway, I think populating fromBucketCollectionIDs on start instead of lazily will simplify things and avoid potential races accessing fromBucketCollectionIDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made a precondition of this that the DataStores are computed when XDCR.Start is called, and that there have to be matching collection names.
We could relax this in a future PR but I don't think this is necessary for testing at this time.
xdcr.go
Outdated
|
||
switch event.Opcode { | ||
case sgbucket.FeedOpDeletion, sgbucket.FeedOpMutation: | ||
if strings.HasPrefix(docID, sgbucket.SyncDocPrefix) && !strings.HasPrefix(docID, sgbucket.Att2Prefix) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to abstract this as a key filter function that's defined on the replication (i.e. XDCR.keyFilterFunc), and set that filter to a pre-defined function when the replication is created with sgbucket.XDCRMobileOn. That would keep the general XDCR implementation a bit more generic and make it clear what's happening when XDCRMobileOn is set.
xdcr.go
Outdated
return true | ||
} | ||
|
||
toCollection, ok := toDataStore.(*Collection) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unexpected to me that we're doing this check after we've already done toDataStore.Get. Can we just get the collection in the first place on line 107?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this code to avoid casts by implementing fixes in sgbucket to consolidate these interfaces into DataStore. This code exists in a separate PR but is included in this PR.
xdcr.go
Outdated
return nil | ||
} | ||
|
||
// writeDoc writes a document to the target datastore. This will not return an error on a CAS mismatch, but will return error on other types of write. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what "other types of write" means in this comment. Also, setWithMeta still needs to do a CAS check to ensure the document hasn't been mutated on the target between the time we evaluated LWW and the time we write, right?
xdcr.go
Outdated
// writeDoc writes a document to the target datastore. This will not return an error on a CAS mismatch, but will return error on other types of write. | ||
func writeDoc(ctx context.Context, collection *Collection, originalCas uint64, event sgbucket.FeedEvent) error { | ||
if event.Opcode == sgbucket.FeedOpDeletion { | ||
_, err := collection.Remove(string(event.Key), originalCas) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think XDCR does delWithMeta to ensure that the delete mutation on the target ends up with the same CAS as the delete mutation on the source - does collection.Remove() do the same thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed.
xdcr.go
Outdated
|
||
err := collection.SetWithMeta(ctx, string(event.Key), originalCas, event.Cas, event.Expiry, xattrs, body, event.DataType) | ||
|
||
if !collection.IsError(err, sgbucket.KeyNotFoundError) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want to ignore a KeyNotFound error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
xdcr.go
Outdated
|
||
} | ||
|
||
err := collection.SetWithMeta(ctx, string(event.Key), originalCas, event.Cas, event.Expiry, xattrs, body, event.DataType) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we preserving system (_sync) xattrs on the target here? Or is that a pending enhancement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All xattrs are preserved. A future enhancement will be to handle _vv
and _mou
xattrs.
Dropping this PR to implement this entirely within sync gateway to avoid three repo PRs. |
_vv, _mou, or _sync handling.
couchbase/sg-bucket#117
Includes #30 which can be merged independently