Skip to content
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

Endblock feature #2064

Merged
merged 15 commits into from
Oct 29, 2023
Merged

Endblock feature #2064

merged 15 commits into from
Oct 29, 2023

Conversation

guplersaxanoid
Copy link
Contributor

Description

Allow an optional endBlock on the datasource. Handlers in that datasource will only run from startBlock to endBlock

Fixes #2008

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

Checklist

  • I have tested locally
  • I have performed a self review of my changes
  • Updated any relevant documentation
  • Linked to any relevant issues
  • I have added tests relevant to my changes
  • Any dependent changes have been merged and published in downstream modules
  • My code is up to date with the base branch
  • I have updated relevant changelogs. We suggest using chan

Copy link
Collaborator

@stwiname stwiname left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good. I think there still needs to be some updates for dictionary queries.

We also need some tests please

packages/node-core/src/indexer/project.service.ts Outdated Show resolved Hide resolved
packages/node-core/src/indexer/project.service.ts Outdated Show resolved Hide resolved
packages/common-substrate/src/project/models.ts Outdated Show resolved Hide resolved
],
],
[201, [{startBlock: 1, endBlock: 300}]],
[301, []],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this can happen then the indexer will exit because of

if (!(await this.projectService.hasDataSourcesAfterHeight(height))) {
logger.info(`All data sources have been processed up to block number ${height}. Exiting gracefully...`);
await this.storeCacheService.flushCache(false, true);
process.exit(0);

We either need to support this correctly in the fetch service when enqueuing blocks or throw an error if this happens

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If no datasources are available to index a specific blockheight we throw this error:

https://github.com/subquery/subql/blob/5c3142bb41f9196e4fbe433ede8f66021dfbf3cc/packages/node-core/src/indexer/indexer.manager.ts#L145C1-L159C4

should we change this behaviour to somehow skip blocks that has no datasources?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that happens too late on having to index to that height to see the error.

I think we can skip blocks, its should be relatively easy to do

[
1,
{
dataSources: [{startBlock: 1}],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pre-existing bug buy if this change is made then it will produce a different result but the expected result is as it is now

Suggested change
dataSources: [{startBlock: 1}],
dataSources: [{startBlock: 1}, {startBlock: 200}],

Comment on lines 315 to 318
const minStartHeight = sortedEvents[0].block;
[...dsMap.entries()].forEach(([height, ds]) => {
if (height >= minStartHeight) dsMap.delete(height);
});
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than deleting, why not look forward to the next project and filter based on that?

@@ -208,6 +208,12 @@ export abstract class BaseBlockDispatcher<Q extends IQueue, DS> implements IBloc
// Flush all data from cache and wait
await this.storeCacheService.flushCache(false, true);
}

if (!(await this.projectService.hasDataSourcesAfterHeight(height))) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is right. What happens if we run into bypassblocks at the next height?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we can do this check with datasources map to make sure we are not exiting prematurely

packages/node-core/src/indexer/fetch.service.ts Outdated Show resolved Hide resolved
@stwiname stwiname merged commit a25ec50 into main Oct 29, 2023
@stwiname stwiname mentioned this pull request Oct 29, 2023
13 tasks
@jamesbayly
Copy link
Contributor

Docs for review subquery/documentation#443

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

endblock on DataSource
3 participants