announce indices for deals (rebased on main) #319
Conversation
| } | ||
|
|
||
| func (w *Wrapper) IndexerAnnounceAllDeals(ctx context.Context) error { | ||
| log.Info("will announce all Boost deals to Indexer") |
There was a problem hiding this comment.
This is not very informative, maybe downgrade to Debug ?
There was a problem hiding this comment.
It's just once per that CLI command. Harmless but nice to have when going through logs
dirkmc
force-pushed
the
nonsense/announce-indices-for-deals
branch
from
53a52f3
to
221c8eb
Compare
|
|
||
| func (w *Wrapper) IndexerAnnounceAllDeals(ctx context.Context) error { | ||
| log.Info("will announce all Markets deals to Indexer") | ||
| err := w.legacyProv.AnnounceAllDealsToIndexer(ctx) |
There was a problem hiding this comment.
We don't seem to be doing anything with that error, maybe check it and have a WARN log message if it is != nil ?
| annCid, err := p.ip.AnnounceBoostDeal(ctx, deal) | ||
| if err != nil && !xerrors.Is(err, provider.ErrAlreadyAdvertised) { | ||
| return fmt.Errorf("failed to announce deal to index provider: %w", err) | ||
| } |
There was a problem hiding this comment.
Shouldn't we check for err != nil (i.e. when the err is provider.ErrAlreadyAdvertised), before saying that deal is successfully announced?
There was a problem hiding this comment.
Hey Anton, if the error is provider.ErrAlreadyAdvertised, we have nothing to worry about and can consider announcement to be successful given how the index provider stuff works.
There was a problem hiding this comment.
Returning both a result and an error is certainly not idiomatic behaviour and somewhat confusing - this is why I mentioned the comment. We basically end up with an error and with the result (annCid), which is really not what one expects unless they know the full stack.
There was a problem hiding this comment.
I see what you mean. Have fixed this.
node/modules/storageminer.go
Outdated
| // We start Boost in this way because we want to ensure Boost is started after the legacy markets Provider. | ||
| // We want to do this so that the index provider callback registered by Boost can overwrite the index | ||
| // provider callback registered by legacy markets as legacy markets does NOT know about Boost storage deals. |
There was a problem hiding this comment.
Is this comment still correct? It looks like we're starting the index provider in HandleIndexProvider
Co-authored-by: Anton Evangelatov <anton.evangelatov@gmail.com>
Should be able to announce indices and serve indices for Boost deals and legacy markets deals.
Markets PR at filecoin-project/go-fil-markets#691.
TODO
AnnounceAllDealsCLI on Sofia Miner.