feat: Make IndexAPI call the database #2201
Conversation
packages/core/src/ceramic.ts
Outdated
| @@ -205,7 +205,6 @@ export class Ceramic implements CeramicApi { | |||
| this._networkOptions = params.networkOptions | |||
| this._loadOptsOverride = params.loadOptsOverride | |||
| this._indexing = modules.indexing | |||
There was a problem hiding this comment.
Related to my comment on the previous PR: now that we pass the indexing into the IndexApi, do we need to store a reference here in the Ceramic class, or can we just keep the reference to this.index and have it be responsible for managing the DatabaseIndexApi?
There was a problem hiding this comment.
Better leave the discussion there, I guess.
| if (this.databaseIndexApi) { | ||
| const page = await this.databaseIndexApi.page(query) | ||
| const streamStates = await Promise.all( | ||
| page.entries.map((streamId) => this.repository.streamState(streamId)) |
There was a problem hiding this comment.
Repository.streamState() doesn't use the loading queue or add the state to the cache. Should we be using Repository.get() instead?
There was a problem hiding this comment.
It was Repository::load first :D before I realised that the users will most probably load hundreds of streams at once. In my opinion, there is a pretty high chance a Ceramic node will never see some stream except in the indexing response. In this case, why make its life harder with loading queues, cache invalidation and so on?
There was a problem hiding this comment.
I more buy the argument that we should skip the loading queue since we know we won't be generating pubsub queries so the need to rate limit is less. I'm a little less convinced that we should skip caching streams that are loaded via queries. I do see the concern though that one large query could blow away your entire cache in one fell swoop. So I'm okay with going with this for now and we can revisit in the future if this causes issues.
| async queryIndex(query: BaseQuery & Pagination): Promise<Page<StreamState>> { | ||
| if (this.databaseIndexApi) { | ||
| const page = await this.databaseIndexApi.page(query) | ||
| const streamStates = await Promise.all( |
There was a problem hiding this comment.
I'm a little worried about us hammering S3 with hundreds of simultaneous requests. I think we might want to break this down into smaller batches and/or rate limit how fast we hammer the state store with these requests
There was a problem hiding this comment.
I believe it is a responsibility of S3StateStore then, and not this thing. Created an issue for that.
ukstv
force-pushed
the
feature/net-1522-implement-indexapi-for-ceramic-core
branch
from
e8947c9 to
2d9b84e
Compare
| if (this.databaseIndexApi) { | ||
| const page = await this.databaseIndexApi.page(query) | ||
| const streamStates = await Promise.all( | ||
| page.entries.map((streamId) => this.repository.streamState(streamId)) |
There was a problem hiding this comment.
I more buy the argument that we should skip the loading queue since we know we won't be generating pubsub queries so the need to rate limit is less. I'm a little less convinced that we should skip caching streams that are loaded via queries. I do see the concern though that one large query could blow away your entire cache in one fell swoop. So I'm okay with going with this for now and we can revisit in the future if this causes issues.
ukstv
force-pushed
the
feature/net-1522-implement-indexapi-for-ceramic-core
branch
from
8cd8182 to
aa6aa91
Compare
ukstv
merged commit 0c8aad4
into
feature/net-1521-expose-indexingapi-as-part-of-ceramicapi
…ent (#2200) * feat: add dummy implementation of IndexClientApi to core and http-client * feat: Rename IndexApi * chore: Different version of the index initialization * feat: Make IndexAPI call the database (#2201) * feat: Make IndexAPI call the database * feat: use Repository::streamState to get StreamState by StreamID * chore: simpler preparation of queryIndex result * Update packages/core/src/indexing/local-index-api.ts Co-authored-by: Spencer T Brody <[email protected]> * chore: After-rebase fixes Co-authored-by: Spencer T Brody <[email protected]> * chore: After-merge fixes Co-authored-by: Spencer T Brody <[email protected]>
Potential controversies:
queryIndexmethod return an empty page. We also log a warning for every query.Repository::streamStateto load StreamState for StreamID. We assume that a state store always contains StreamState for an indexed stream.Base branch is not
developmentto reduce mental overhead for reviewing.