CBG-3271 make sure expiration works on a closed bucket #17
Conversation
Make sure the following works: - open bucket - write doc with expiry of 1 sec - close bucket - wait more than 1 sec The doc should expire, because another caller can re-open foo, and would expect the document to be deleted. This is akin to metadata purge interval. Implementation: - Abstracted ExpirationManager into a separate struct / file for readability. - ExpirationManager exists on every Bucket, since expiration is set based on a CRUD operation, which could occur on any copy of the bucket. - The functions for expiration use a new function _underlyingDB which returns a queryable object that is never closed.
bucket.go
Outdated
| @@ -203,7 +202,7 @@ func OpenBucket(urlStr string, bucketName string, mode OpenMode) (b *Bucket, err | |||
| } | |||
|
|
|||
| registerBucket(bucket) | |||
| return bucket, err | |||
| return bucket.copy(), err | |||
There was a problem hiding this comment.
What happens in the case of concurrent OpenBucket requests for a bucket that doesn't already exist in the registry? It doesn't look like we're doing any locking, so in the case that registerBucket finds an existing bucket in the registry at line 205 (i.e. this instance of OpenBucket lost the race), I think we should be returning a copy of that bucket, and not the newly created one.
| } | ||
|
|
||
| // _scheduleExpirationAtOrBefore schedules the next expiration of documents to occur, from the minimum expiration value in the bucket. Requires the mutext to be held. | ||
| func (e *expiryManager) _scheduleExpirationAtOrBefore(exp uint32) { |
There was a problem hiding this comment.
It doesn't look like we actually need this version (that requires holding the mutex, based on my other comment) - can this just be moved inside scheduleExpirationAtOrBefore?
close a duplicate bucket if two callers are using OpenBucket at the same time
bucket_registry.go
Outdated
| } | ||
|
|
||
| // registryNewBucket adds a newly opened Bucket to the registry. | ||
| func registerNewBucket(bucket *Bucket) { |
There was a problem hiding this comment.
This doesn't look like it's needed any longer?
bucket.go
Outdated
| bucket.Close(ctx) | ||
| } | ||
| }() | ||
|
|
||
| if mode == CreateNew { |
There was a problem hiding this comment.
Do you think we should pass OpenMode to getCachedBucket to avoid some of the open-then-close we're doing?
| if mode == CreateNew { | ||
| return nil, fs.ErrExist | ||
| } | ||
| if urlStr != bucket.url { | ||
| return nil, fmt.Errorf("bucket %q already exists at %q, will not open at %q", bucketName, bucket.url, urlStr) | ||
| } | ||
| registerBucket(bucket) | ||
| return bucket, nil | ||
|
|
||
| } |
There was a problem hiding this comment.
Does the handling on line 117 to not support ReOpenExisting for inMemory buckets still make sense with the bucket registry/cluster?
bucket_registry.go
Outdated
| // registryBucket adds a newly opened Bucket to the registry. | ||
| func (r *bucketRegistry) registerBucket(bucket *Bucket) { | ||
| // registryBucket adds a newly opened Bucket to the registry. Returns true if the bucket already exists, and a copy of the bucket to use. | ||
| func (r *bucketRegistry) registerBucket(bucket *Bucket) (bool, *Bucket) { | ||
| name := bucket.GetName() | ||
| debug("registerBucket %v %s at %s", bucket, name, bucket.url) |
There was a problem hiding this comment.
This logging is done in _registerBucket, looks like you could remove it here.
bucket_registry.go
Outdated
| @@ -35,17 +35,26 @@ func init() { | |||
| } | |||
| } | |||
|
|
|||
| // registryBucket adds a newly opened Bucket to the registry. | |||
| func (r *bucketRegistry) registerBucket(bucket *Bucket) { | |||
| // registryBucket adds a newly opened Bucket to the registry. Returns true if the bucket already exists, and a copy of the bucket to use. | |||
There was a problem hiding this comment.
| // registryBucket adds a newly opened Bucket to the registry. Returns true if the bucket already exists, and a copy of the bucket to use. | |
| // registerBucket adds a newly opened Bucket to the registry. Returns true if the bucket already exists, and a copy of the bucket to use. |
Make sure the following works:
The doc should expire, because another caller can re-open foo, and would expect the document to be deleted. This is akin to metadata purge interval.
Implementation:
Questions:
I really don't like
TestExpirationAfterClosebecause it adds aSleepto ensure it doesn't panic, and it doubles the test time, which is mostly slow already becauseTestExpirationhas a 3 second sleep. I can't think of a good way to shorten either test, but I thinkTestExpirationAfterClosemight be removed for non development since it repros pretty easily in sync gateway CI.