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

Move more functionality into sg-bucket interface and avoid type assertions #2418

Closed
tleyden opened this issue Mar 22, 2017 · 5 comments
Closed
Assignees
Milestone

Comments

@tleyden
Copy link
Contributor

tleyden commented Mar 22, 2017

Move GetMaxVbno() into sg-bucket interface

This code is unnecessarily brittle:

	log.Printf("indexWriteBucket: %+v type: %T", k.indexWriteBucket, k.indexWriteBucket)
	cbBucket, ok := k.indexWriteBucket.(base.CouchbaseBucketGoCB)
	var maxVbNo uint16
	if ok {
		maxVbNo, err = cbBucket.GetMaxVbno()
		if err != nil {
			base.Warn("Unable to obtain vbucket count from cluster - defaulting to 1024")
			maxVbNo = 1024
		}
	} else {
		// walrus, for unit testing
		base.Warn("Not a couchbase bucket - assuming walrus, setting maxVbNo=1024")
		maxVbNo = 1024
	}

If k.indexWriteBucket is a *base.CouchbaseBucketGoCB rather than a base.CouchbaseBucketGoCB, then this code will break. So far it looks like that's what's behind couchbaselabs/mobile-testkit#1072

If GetMaxVbno() was defined on the sg-bucket bucket interface and all buckets implemented it, it feels like it would be more robust.

@adamcfraser
Copy link
Collaborator

I'm not against the idea of adding GetMaxVbno() to the bucket interface, but I think the check here may have just been implemented incorrectly - I think should have been this all along:
cbBucket, ok := k.indexWriteBucket.(*base.CouchbaseBucketGoCB)

@tleyden
Copy link
Contributor Author

tleyden commented Mar 23, 2017

This also should be moved into sg-bucket interface:

Add to the bucket interface: getUUID()

sgw_pindex.go

cbBucket, ok := bucket.(base.CouchbaseBucket)
	if ok {
		log.Printf("is base.CouchbaseBucket, call GetCBGTIndexName")
		indexName = GetCBGTIndexName(cbBucket)
	} else {
		log.Printf("not base.CouchbaseBucket, call bucket.GetName()")
		indexName = bucket.GetName()
	}
	return indexName

@tleyden tleyden changed the title Move GetMaxVbno() into sg-bucket interface Move more functionality into sg-bucket interface and avoid type assertions Mar 23, 2017
@djpongh djpongh added this to the 2.0.0 milestone Mar 24, 2017
@djpongh djpongh added the ready label Mar 24, 2017
@djpongh
Copy link

djpongh commented Mar 24, 2017

Make sure all functional test post merge.

@tleyden
Copy link
Contributor Author

tleyden commented Mar 29, 2017

Add to the bucket interface: getUUID()

I'm not seeing an easy way to get the bucket UUID via gocb. I posted a question on the go sdk forum:

https://forums.couchbase.com/t/is-it-possible-to-get-the-bucket-uuid/12267

@tleyden
Copy link
Contributor Author

tleyden commented Apr 3, 2017

I'm looking at all the places where we need to do type assertions, and will list the ones that look like decent candidates for moving into the sg-bucket interface.

GetStatsVbSeqno

If this method:

_, highSeqnos, err := cbBucket.GetStatsVbSeqno(maxVbNo, false)

is moved into the sg-bucket interface, then this type assertion could be removed:

// Make sure that the index bucket and data bucket have correct sequence parity
// https://github.com/couchbase/sync_gateway/issues/1133
func VerifyBucketSequenceParity(indexBucketStableClock SequenceClock, baseBucket Bucket) error {

	bucket, err := GetCouchbaseBucketFromBaseBucket(baseBucket)
	if err != nil {
		Warn(fmt.Sprintf("Bucket is a %T not a base.CouchbaseBucket, skipping verifyBucketSequenceParity()\n", bucket))
		return nil
	}

	return VerifyBucketSequenceParityCouchbaseBucket(indexBucketStableClock, bucket)

}

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

No branches or pull requests

3 participants