Rework poi #1966

jiqiang90 · 2023-08-22T04:20:13Z

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes #1905

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
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

packages/common/src/constants.ts

packages/node-core/src/indexer/blockDispatcher/base-block-dispatcher.ts

packages/node-core/src/indexer/poi/poi.service.ts

stwiname · 2023-08-24T01:41:31Z

packages/node-core/src/indexer/poi/poi.service.ts

+    this.isSyncing = true;
+    while (!this.isShutdown) {
+      if (!this._latestSyncedPoi) {
+        await delay(10);


Whats this delay for?

If _latestSyncedPoi is undefined, which means we haven't sync any Poi yet, no genesis poi been found. It delay for genesis poi to be created, delay will stop after ensureGenesisPoi,

subql/packages/node-core/src/indexer/poi/poi.service.ts

Line 160 in bfe6de0

async ensureGenesisPoi(height: number) {

I don't think we should have random delays like this. Its near impossible to follow code with things like that. e.g. if for some reason ensureGenesisPoi fails or a refactor means its not called we could be scratching our heads for hours trying to figure out why this isn't running.

We're better off calling functions in the correct order

Genesis Poi need to create one time only.
Once in metadata stored latestSyncedPoi and in the init will set this object, the sync process will immediately start.

Before I think we could make sure sync process start only after when the non-null operation block been processed, rather than checking every 10 seconds, but we also could face an issue here:
Assume we restart the app, in current poi, latestSyncedPoi is 1000, and we have poi 1002,1005 awaiting to be synced.
Then after 1005 ,there is no blocks with operation. which means 1002 and 1005 will be never been synced..

In current implementation will solve this issue.

I will leave a comment with this method

packages/node-core/src/indexer/poi/poi.service.ts

stwiname · 2023-08-24T02:11:35Z

packages/node-core/src/indexer/poi/poi.service.ts

+      }
+      // Remove and Change column from sequelize not work, it only applies to public schema
+      // https://github.com/sequelize/sequelize/issues/13365
+      // await this.poiRepo?.model.sequelize?.getQueryInterface().changeColumn(tableName,'mmrRoot',{})


Can we run raw sql?

Current implementation is using raw sql, I left this comment here due to this bug here, we could not alter poi table columns with sequelize method getQueryInterface().changeColumn

So the code above this is used instead of this?

packages/node-core/src/indexer/poi/poi.service.ts

stwiname · 2023-08-24T02:16:57Z

packages/node-core/src/indexer/poi/poi.service.ts

+    for (let i = this.latestSyncedPoi.id + 1; i <= endHeight; i++) {
+      const syncedPoiBlock = PoiBlock.create(i, null, null, this.projectId, this.latestSyncedPoi.hash);
+      appendedBlocks.push(syncedPoiBlock);
+      void this.setLatestSyncedPoi(syncedPoiBlock);


Why is this void?

the setLatestSyncedPoi provide any option to force flush the cache, so we only await if we need to force flush.

When we create default poi blocks with a continuous range height, we can just use void here.

Could we just set it at the end of the for loop?

it get parentHash from this.latestSyncedPoi, so it need to be set at every single block

stwiname · 2023-08-24T03:02:45Z

It would be good for Poi Service to have coverage over 80%

poi creation add indexes tidy up fix index query tidy up logger, workers and reindex clean mmr clean mmr test and add validation remove logs drop mmr table fix up fix up test Update packages/common/src/constants.ts Co-authored-by: Scott Twiname <[email protected]> Update packages/node-core/src/indexer/poi/poi.service.ts Co-authored-by: Scott Twiname <[email protected]> Update packages/node-core/src/indexer/poi/poi.service.ts Co-authored-by: Scott Twiname <[email protected]> Update packages/node-core/src/indexer/poi/poi.service.ts Co-authored-by: Scott Twiname <[email protected]> Update packages/node-core/src/indexer/poi/poi.service.ts Co-authored-by: Scott Twiname <[email protected]> catch error and exit fix add more test

stwiname

It would be good to have some documentation of how POI works, maybe just some comments in PoiBlock.ts

The other concern I have is that we're computing POI for blocks that have no operations. Optimism/Arbitrum for example have 100M+ blocks. That is a large amount of data to store just for POI. This is due to wanting to query the POI at any block height.

Is there something better we could do here? Like:

Say the POI for any height is the same as the last height there was an operation
Compute the POI at a specific height when requested as opposed to storing it and it never being queried.
- We could also have checkpoints every N blocks so we don't have to compute too much

packages/node-core/src/indexer/poi/poi.service.ts

packages/node-core/src/indexer/poi/PoiBlock.ts

stwiname · 2023-08-30T23:49:24Z

packages/node-core/src/indexer/poi/poi.service.ts

+      }
+      // Remove and Change column from sequelize not work, it only applies to public schema
+      // https://github.com/sequelize/sequelize/issues/13365
+      // await this.poiRepo?.model.sequelize?.getQueryInterface().changeColumn(tableName,'mmrRoot',{})


So the code above this is used instead of this?

stwiname · 2023-08-31T04:11:20Z

packages/node-core/src/indexer/poi/poi.service.ts

+  constructor(
+    private storeCache: StoreCacheService,
+    private eventEmitter: EventEmitter2,
+    private project: ISubqueryProject


You can use @Inject('ISubqueryProject') here instead of having to use a factory

jiqiang90 marked this pull request as ready for review August 23, 2023 07:47

jiqiang90 force-pushed the rework-poi branch 2 times, most recently from 9c20519 to bfe6de0 Compare August 23, 2023 09:24

stwiname reviewed Aug 24, 2023

View reviewed changes

jiqiang90 force-pushed the rework-poi branch from a441988 to 7542a0c Compare August 29, 2023 01:09

jiqiang90 force-pushed the rework-poi branch from 7542a0c to 2af3687 Compare August 29, 2023 02:30

fix

22ea168

jiqiang90 requested review from stwiname August 29, 2023 02:58

jiqiang90 added 2 commits August 29, 2023 15:09

lint

6a7ee02

fix test

ccdbac0

stwiname reviewed Aug 30, 2023

View reviewed changes

jiqiang90 added 2 commits August 31, 2023 15:42

update

cc2c341

comment

61e5921

stwiname reviewed Aug 31, 2023

View reviewed changes

jiqiang90 added 2 commits August 31, 2023 16:19

use injection

a471408

private

d22bcce

stwiname approved these changes Aug 31, 2023

View reviewed changes

jiqiang90 merged commit 3dff433 into main Aug 31, 2023
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework poi #1966

Rework poi #1966

jiqiang90 commented Aug 22, 2023 •

edited

Loading

stwiname Aug 24, 2023

jiqiang90 Aug 24, 2023

stwiname Aug 24, 2023

jiqiang90 Aug 28, 2023

stwiname Aug 24, 2023

jiqiang90 Aug 24, 2023

stwiname Aug 30, 2023

stwiname Aug 24, 2023

jiqiang90 Aug 24, 2023

stwiname Aug 24, 2023

jiqiang90 Aug 24, 2023

stwiname commented Aug 24, 2023

stwiname left a comment

stwiname Aug 30, 2023

stwiname Aug 31, 2023

Rework poi #1966

Rework poi #1966

Conversation

jiqiang90 commented Aug 22, 2023 • edited Loading

Description

Type of change

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stwiname commented Aug 24, 2023

stwiname left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jiqiang90 commented Aug 22, 2023 •

edited

Loading