Skip to content

Conversation

frrist
Copy link
Member

@frrist frrist commented Feb 9, 2022

What

This PR targets the indexer-refactor branch.
This PR aims to keep indexer behavior consistent with the current while landing some refactor work to improve code extensibility.

  • embed GetExecutedAndBlockMessagesForTipset in processors, preventing unnecessary (and expensive!) calls to the method when its output isn't used. And example of this would be running all actor tasks excluding the miner actor. Prior changes here ensure multiple calls to this method are only executed once, returning a cached value for subsequent calls. This change also makes the indexer's logic more concise.
  • embed GetMessageExecutions in processors, taking advantage of the memoized method pattern used in previous commit. This has the added bonus of removing the ProcessMessageExecution interface and fitting the messege execution task under the ProcessMessages interface.

Follow on: #871

@codecov-commenter
Copy link

codecov-commenter commented Feb 9, 2022

Codecov Report

❌ Patch coverage is 26.02740% with 54 lines in your changes missing coverage. Please review.
✅ Project coverage is 32.6%. Comparing base (96f48ac) to head (9c47558).

Additional details and impacted files
@@                Coverage Diff                 @@
##           indexer-refactor    #850     +/-   ##
==================================================
+ Coverage              32.1%   32.6%   +0.4%     
==================================================
  Files                    39      39             
  Lines                  3785    3715     -70     
==================================================
- Hits                   1217    1212      -5     
+ Misses                 2420    2356     -64     
+ Partials                148     147      -1     
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@frrist frrist force-pushed the frrist/indexer-cleanup-part1 branch 2 times, most recently from 37bb4cd to 3b14800 Compare February 9, 2022 23:46
Comment on lines 41 to 46
mex, err := p.node.GetMessageExecutionsForTipSet(ctx, ts, pts)
if err != nil {
report.ErrorsDetected = xerrors.Errorf("getting messages executions for tipset: %w", err)
return nil, report, nil
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Returning the error here would be fatal meaning the corresponding walk, watch, or fill job would abort completely -- that would be inconsistent with current behavior. So, we list this as a detected error in the report which will mark the task as ERROR in the corresponding processing report, and return immediately with no data extracted.

Comment on lines 50 to 54
tsMsgs, err := p.node.GetExecutedAndBlockMessagesForTipset(ctx, ts, pts)
if err != nil {
report.ErrorsDetected = xerrors.Errorf("getting executed and block messages: %w", err)
return nil, report, nil
}
Copy link
Member Author

Choose a reason for hiding this comment

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

(ditto) Returning the error here would be fatal meaning the corresponding walk, watch, or fill job would abort completely -- that would be inconsistent with current behavior. So, we list this as a detected error in the report which will mark the task as ERROR in the corresponding processing report, and return immediately with no data extracted.

Comment on lines 49 to 55
tsMsgs, err := p.node.GetExecutedAndBlockMessagesForTipset(ctx, ts, pts)
if err != nil {
report.ErrorsDetected = xerrors.Errorf("getting executed and block messages: %w", err)
return nil, report, nil
}
emsgs := tsMsgs.Executed

Copy link
Member Author

Choose a reason for hiding this comment

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

(ditto) Returning the error here would be fatal meaning the corresponding walk, watch, or fill job would abort completely -- that would be inconsistent with current behavior. So, we list this as a detected error in the report which will mark the task as ERROR in the corresponding processing report, and return immediately with no data extracted.

@frrist frrist mentioned this pull request Feb 10, 2022
Base automatically changed from frrist/task-api to indexer-refactor February 16, 2022 18:05
@frrist frrist force-pushed the frrist/indexer-cleanup-part1 branch from 3b14800 to beba785 Compare February 16, 2022 18:21
- merge message execution processor with message processor
- use DataSource API to retrieve Internal, Executed, and Block
  messages from within their respective processors
@frrist frrist force-pushed the frrist/indexer-cleanup-part1 branch from beba785 to 9c47558 Compare February 16, 2022 18:26
@frrist frrist marked this pull request as ready for review February 16, 2022 18:38
@frrist frrist requested review from iand and placer14 February 16, 2022 18:41
@frrist frrist changed the title refactor chain indexer part1 setup for indexer refacor Feb 16, 2022
Copy link
Contributor

@iand iand left a comment

Choose a reason for hiding this comment

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

LGTM. One step closer to simplifying the indexer

@frrist frrist merged this pull request into indexer-refactor Feb 22, 2022
@frrist frrist deleted the frrist/indexer-cleanup-part1 branch February 22, 2022 21:04
@frrist frrist mentioned this pull request Feb 22, 2022
3 tasks
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.

3 participants