-
Notifications
You must be signed in to change notification settings - Fork 5
Add a storetheindex delegated provider #158
Changes from all commits
fc37576
f8eaf00
6db49e3
172615e
73f8538
ef5b8a2
6a1c2ac
222ad10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,58 +8,49 @@ import ( | |
"github.com/libp2p/go-libp2p-kad-dht/providers" | ||
"github.com/libp2p/hydra-booster/metrics" | ||
"go.opencensus.io/stats" | ||
"go.opencensus.io/tag" | ||
) | ||
|
||
// CachingProviderStore wraps a providerstore, and finds providers for CIDs that were requested | ||
// but not found in the underlying providerstore, and then caches them in the providerstore. | ||
// CachingProviderStore checks the ReadProviderStore for providers. If no providers are returned, | ||
// then the Finder is used to find providers, which are then added to the WriteProviderStore. | ||
type CachingProviderStore struct { | ||
Delegate providers.ProviderStore | ||
Finder ProvidersFinder | ||
Router ReadContentRouting | ||
ProvidersToFind int | ||
log logging.EventLogger | ||
ReadProviderStore providers.ProviderStore | ||
WriteProviderStore providers.ProviderStore | ||
Finder ProvidersFinder | ||
Router ReadContentRouting | ||
log logging.EventLogger | ||
} | ||
|
||
func NewCachingProviderStore(delegate providers.ProviderStore, finder ProvidersFinder, router ReadContentRouting) *CachingProviderStore { | ||
func NewCachingProviderStore(getDelegate providers.ProviderStore, addDelegate providers.ProviderStore, finder ProvidersFinder, router ReadContentRouting) *CachingProviderStore { | ||
return &CachingProviderStore{ | ||
Delegate: delegate, | ||
Finder: finder, | ||
Router: router, | ||
log: logging.Logger("hydra/providersn"), | ||
ReadProviderStore: getDelegate, | ||
WriteProviderStore: addDelegate, | ||
Finder: finder, | ||
Router: router, | ||
log: logging.Logger("hydra/providers"), | ||
} | ||
} | ||
|
||
func (s *CachingProviderStore) AddProvider(ctx context.Context, key []byte, prov peer.AddrInfo) error { | ||
return s.Delegate.AddProvider(ctx, key, prov) | ||
return s.WriteProviderStore.AddProvider(ctx, key, prov) | ||
} | ||
|
||
// GetProviders gets providers for the given key from the providerstore. | ||
// If the providerstore does not have providers for the key, then the ProvidersFinder is queried and the results are cached. | ||
func (d *CachingProviderStore) GetProviders(ctx context.Context, key []byte) ([]peer.AddrInfo, error) { | ||
addrInfos, err := d.Delegate.GetProviders(ctx, key) | ||
addrInfos, err := d.ReadProviderStore.GetProviders(ctx, key) | ||
if err != nil { | ||
return addrInfos, err | ||
} | ||
|
||
if len(addrInfos) > 0 { | ||
recordPrefetches(ctx, "local") | ||
return addrInfos, nil | ||
} | ||
|
||
return nil, d.Finder.Find(ctx, d.Router, key, func(ai peer.AddrInfo) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably not worth the effort, but for future reference. It is bad style to reuse the error return value for two different purposes. Above, returned errors indicate errors in the providerstores. Here, errors indicate errors in the finder. Considering that the finder is an async functionality, independent of the GetProviders execution path, its error should be logged, not returned. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Otherwise this function seems to be a correct implementation of the logic we discussed in colo. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't heard of that style guideline before, if a method is supposed to do two things, and it can't do one of them, then it's normal to return an error, regardless of which one failed. The finder is best effort, so an error from the finder here doesn't represent an error in the finding itself, it means that we couldn't even try to find (e.g. in async case, that the work couldn't be queued for some reason). My intention was for the CachingProviderStore to not know nor care that things are happening async, but given that we don't have a sync version of this, I can see how that just adds confusion, so I can rename things a bit here to clarify. |
||
err := d.Delegate.AddProvider(ctx, key, ai) | ||
err := d.WriteProviderStore.AddProvider(ctx, key, ai) | ||
if err != nil { | ||
d.log.Errorf("failed to add provider to providerstore: %s", err) | ||
stats.Record(ctx, metrics.PrefetchFailedToCache.M(1)) | ||
} | ||
}) | ||
} | ||
|
||
func recordPrefetches(ctx context.Context, status string, extraMeasures ...stats.Measurement) { | ||
stats.RecordWithTags( | ||
ctx, | ||
[]tag.Mutator{tag.Upsert(metrics.KeyStatus, status)}, | ||
append([]stats.Measurement{metrics.Prefetches.M(1)}, extraMeasures...)..., | ||
) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,7 +80,9 @@ func mergeAddrInfos(infos []peer.AddrInfo) []peer.AddrInfo { | |
} | ||
var r []peer.AddrInfo | ||
for k, v := range m { | ||
r = append(r, peer.AddrInfo{ID: k, Addrs: v}) | ||
if k.Validate() == nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems fine, but why wouldn't we do this check further up next to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. because we aren't doing an explicit iteration through the addrInfo's / keys during that part of accumulation |
||
r = append(r, peer.AddrInfo{ID: k, Addrs: v}) | ||
} | ||
} | ||
return r | ||
} | ||
|
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.
I think this sequence of if statements is correct, but for future reference: When your intention is to have mutually-exclusive cases, both for readability and safety, it is best to capture them either as a switch statement, or with "if else" chain. Here switch statement would be best:
switch {
case cfg.ProvidersFinder != nil && cfg.StoreTheIndexAddr == "":
...
break
case cfg.ProvidersFinder != nil && cfg.StoreTheIndexAddr != "":
...
break
case cfg.ProvidersFinder == nil && cfg.StoreTheIndexAddr != "":
...
break
default:
... something is not right ...
}