CBG-4980: support /db/_flush for internal testing #7864
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds test coverage for the flush operation with both transient and persistent configuration modes, ensuring compatibility with Rosmar (the in-memory bucket implementation).
Key changes:
- Parameterized the
TestFlushtest to run with both persistent and non-persistent configurations - Added explicit database creation step when using persistent config
- Removed redundant
SetAdminPartycall after flush operation - Added verification step to recreate and retrieve documents after flush
Redocly previews |
torcolvin
changed the title
adamcfraser
left a comment
adamcfraser
left a comment
There was a problem hiding this comment.
Generally looks good, a few questions on some of the specifics.
base/collection.go
Outdated
| return nil, fmt.Errorf("bucket is not a gocb bucket (type %T)", baseBucket) | ||
| } | ||
|
|
||
| // AsRosmar returns a bucket as a rosmar.Bucket, or an error if it is not one. |
There was a problem hiding this comment.
It feels odd to stick a single *rosmar.Bucket helper function in the middle of the GocbV2Bucket implementation of BucketStore (and related interfaces), but I suppose there isn't really a natural location for it elsewhere? I might prefer it in rosmar_cluster just for the grouping of rosmar functionality, even if it's not cluster-related? If you feel it belongs where you've got it I'm ok with that, if it was an intentional choice.
There was a problem hiding this comment.
Yeah, this I'll move this. I kept it close by to AsGocbV2Bucket but I think it is weird too.
base/rosmar_cluster.go
Outdated
| if runtime.GOOS == "windows" { | ||
| directory = strings.TrimPrefix(directory, "/") | ||
| } | ||
| err = os.MkdirAll(directory, 0750) |
There was a problem hiding this comment.
Why are group permissions needed here? (0750 vs 0700)
There was a problem hiding this comment.
Fixed, no good reason.
rest/api.go
Outdated
| if err2 != nil { | ||
| return err2 | ||
| defer bucket.Close(h.ctx()) | ||
| // Flush the bucket (assuming it conforms to sgbucket.DeleteableStore interface |
There was a problem hiding this comment.
It doesn't look like we're using the DeleteableStore interface check any more?
There was a problem hiding this comment.
Good call, this actually requires Flush anyway.
CBG-4980 support /db/_flush for internal testing
Also addresses:
NOTE: _flush is a database call, so any other databases that are backed by the same bucket will be destroyed.
AddDatabaseFromConfig. It would also not block database polling while this happened which could result in a crash.Behavior changes:
InsertConfigorAddDatabaseFromConfig. Test to make sure config polling no-ops after calling /db/_flushOpenBucketIninRosmarCluster. When usingrosmar.InMemoryURL,OpenBucketandOpenBucketInbehave identically, but they do not otherwise.rosmar.Bucket.DefaultDataStore()would just return nil, and we didn't do a nil check. Probably https://github.com/couchbaselabs/rosmar/blob/3b9ac157a3cd52c076649d31cbe385e76386787d/bucket_api.go#L122 should panic, or we change thesg-bucket.Bucket.DefaultDataStoreAPI to return errors. I thought this was out of scope for this PR, and we might want to backport this to older releases where we might not want to switch rosmar.When flushing or deleting a bucket, you need to create a copy of the bucket object.
Possible Different Approach
Since
/db/_flushis database scoped, but it flushes the entire bucket, this means other databases that are backed by the same bucket are deleted. I have considered removing this API entirely, and maybe replacing it with an API that is like/_internal/rosmar/bucketNamethat could delete a bucket that is either in memory or on disk. This would be more clear in the behavior but add an API that we don't to use outside of some E2E testing.Integration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGatewayIntegration/156/