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-2201: Added LeakyBucket support to the rest tester under integration tests #5679

Merged
merged 10 commits into from Aug 25, 2022

Conversation

IsaacLambat
Copy link
Member

@IsaacLambat IsaacLambat commented Aug 5, 2022

CBG-2201

  • Added LeakyBucket support to the Rest Tester for running with integration tests when using config option
  • Added collection support to the rest tester when using leaky buckets
  • A package will now panic if a bucket is left unclosed instead of just logging a warning

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

Integration Tests

@IsaacLambat IsaacLambat self-assigned this Aug 5, 2022
@IsaacLambat IsaacLambat force-pushed the CBG-2201 branch 3 times, most recently from 59b81c4 to 65609e4 Compare August 5, 2022 16:51
@IsaacLambat IsaacLambat assigned bbrks and unassigned IsaacLambat Aug 5, 2022
rest/server_context.go Outdated Show resolved Hide resolved
rest/utilities_testing.go Outdated Show resolved Hide resolved
rest/utilities_testing.go Outdated Show resolved Hide resolved
rest/utilities_testing.go Outdated Show resolved Hide resolved
rest/server_context.go Outdated Show resolved Hide resolved
db/database.go Outdated Show resolved Hide resolved
rest/utilities_testing.go Outdated Show resolved Hide resolved
base/util_testing.go Outdated Show resolved Hide resolved
rest/utilities_testing.go Outdated Show resolved Hide resolved
rest/utilities_testing.go Show resolved Hide resolved
@bbrks
Copy link
Member

bbrks commented Aug 11, 2022

@IsaacLambat

Can you take a look at the build failure? Probably a break after rebasing or something

[2022-08-11T01:08:51.702Z] # github.com/couchbase/sync_gateway/base
[2022-08-11T01:08:51.702Z] base/collection.go:1085:6: AsCollection redeclared in this block
[2022-08-11T01:08:51.702Z] 	base/collection.go:181:6: other declaration of AsCollection
[2022-08-11T01:08:51.702Z] base/collection.go:1100:9: cannot use AsCollection(underlyingBucket) (value of type bool) as type error in return statement:
[2022-08-11T01:08:51.702Z] 	bool does not implement error (missing Error method)

@IsaacLambat IsaacLambat assigned bbrks and adamcfraser and unassigned IsaacLambat and bbrks Aug 22, 2022
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 questions on how we're switching from default to named collection. I'm also wondering if this approach will be compatible with future changes to support multiple collections in a single RestTester context?

}
}
if unclosedBucketWarnings != "" {
panic(unclosedBucketWarnings)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't recall the background for this change - is this to better catch this scenario pre-merge?

Copy link
Member Author

Choose a reason for hiding this comment

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

Originally, this was put in when a Leaky TestBucket was to be passed in to the RT in-order to make sure the bucket was closed manually at the end of the test and can't be missed. I left this change in (even though a TB is no longer passed in) because it would be useful in general as it is quite easy to miss a bucket being left unclosed when not going through all the test logs.

rest/utilities_testing.go Outdated Show resolved Hide resolved

collectionBucket.Spec.Scope = scope
collectionBucket.Spec.Collection = collection
collectionBucket.Collection = collectionBucket.Collection.Bucket().Scope(*scope).Collection(*collection)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What exactly is going on here? testBucket.Bucket is initialized as a gocb Collection targeting the default collection, and if we have a named collection we're using the associated Bucket() to swap to the underlying collection to the named collection?
Are there any allocations associated with the default collection that need to be cleaned up? I know Collection.Close() doesn't really do any work, but it feels a bit unusual to assign collectionBucket.Collection twice in this setup.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are no extra allocations done when changing the scope and collection to a named scope and collection. This is because the GoCB Scope and Collection functions return the provided bucket wrapped in a GoCB Scope and Collection struct.

@IsaacLambat
Copy link
Member Author

A few questions on how we're switching from default to named collection. I'm also wondering if this approach will be compatible with future changes to support multiple collections in a single RestTester context?

The change to add scopes and collections when using a leaky bucket is a temporary measure that can be ripped out once tests are backed by collections.

@adamcfraser adamcfraser merged commit e5ce35c into master Aug 25, 2022
@adamcfraser adamcfraser deleted the CBG-2201 branch August 25, 2022 17:25
markspolakovs added a commit that referenced this pull request Aug 30, 2022
torcolvin pushed a commit that referenced this pull request Sep 1, 2022
…ion tests (#5679)

* CBG-2201: Added LeakyBucket support to the rest tester under integration tests

* Fixed normal buckets double closing causing panic

* Added RT option to use leaky bucket on DB

* Added panic is not using a leaky bucket£

* Addressed PR feedback, change add database function with connect fn to AddDatabaseFromConfigWithBucket

* Added collection support, and use TB on DB if provided in RT if provided

* Address PR feedback

* Rest tester makes leaky bucket when needed

* Modified TestBlipGetCollections to pick up new leaky bucket changes

* Removed createScopesAndCollections
markspolakovs pushed a commit that referenced this pull request Sep 20, 2022
…ion tests (#5679)

* CBG-2201: Added LeakyBucket support to the rest tester under integration tests

* Fixed normal buckets double closing causing panic

* Added RT option to use leaky bucket on DB

* Added panic is not using a leaky bucket£

* Addressed PR feedback, change add database function with connect fn to AddDatabaseFromConfigWithBucket

* Added collection support, and use TB on DB if provided in RT if provided

* Address PR feedback

* Rest tester makes leaky bucket when needed

* Modified TestBlipGetCollections to pick up new leaky bucket changes

* Removed createScopesAndCollections
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.

None yet

3 participants