-
Notifications
You must be signed in to change notification settings - Fork 226
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
fix(providers): gc #325
fix(providers): gc #325
Conversation
Otherwise, we'll delete everything.
@@ -31,7 +31,7 @@ type ProviderManager struct { | |||
// all non channel fields are meant to be accessed only within | |||
// the run method | |||
providers *lru.LRU | |||
dstore ds.Datastore | |||
dstore *autobatch.Datastore |
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 concerned about the lack of thread-safety in the autobatching datastore?
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.
We're only using it from one thread here.
@@ -193,8 +193,7 @@ func writeProviderEntry(dstore ds.Datastore, k cid.Cid, p peer.ID, t time.Time) | |||
|
|||
func (pm *ProviderManager) gc() { | |||
res, err := pm.dstore.Query(dsq.Query{ | |||
KeysOnly: true, |
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.
ok, that's a subtle little change. What's the difference 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.
The expiration is in the value. We had this before because we'd:
- Determine that all records have expired.
- Delete all records.
This caused us to remember a bunch of expired records indefinitely.
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.
OUCH!
Otherwise, we'll delete everything.