-
Notifications
You must be signed in to change notification settings - Fork 179
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
[Execution] Add ingestion throttle #5337
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5337 +/- ##
==========================================
+ Coverage 55.63% 56.15% +0.52%
==========================================
Files 1037 824 -213
Lines 101339 81121 -20218
==========================================
- Hits 56376 45555 -10821
+ Misses 40624 31988 -8636
+ Partials 4339 3578 -761
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
dd61789
to
b118353
Compare
b118353
to
b9d994a
Compare
b9d994a
to
669e087
Compare
return err | ||
} | ||
} else { | ||
unexecuted, err = findFinalized(c.state, c.headers, c.executed, c.executed+500) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
c.executed+500
is 500 supposed to be b.threshold
or just a different constant value we use for the range? If the latter, please make it a const with some comments about how it's used
}, nil | ||
} | ||
|
||
func (c *BlockThrottle) Init(processables chan<- flow.Identifier) error { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO if the block throttle is not inited, or executed > finalized
I would just make the block throttle behave as if it isn't there instead of producing errors.
What do you think?
// CatchUpThreshold is the number of blocks that if the execution is far behind | ||
// the finalization then we will only lazy load the next unexecuted finalized | ||
// blocks until the execution has caught up | ||
const CatchUpThreshold = 500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the default value?
const CatchUpThreshold = 500 | |
const DefaultCatchUpThreshold = 500 |
defer c.mu.Unlock() | ||
|
||
if !c.inited { | ||
return fmt.Errorf("throttle not inited") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really an error scenario? Wont the BlockThrottle
just get inited later in which case we can continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to make sure c.processables
is not nil
, which is supposed to be updated already by the Init
function. If we continue, it will panic with nil error.
But I will refactor to turn inited
into a function and just check if c.processables == nil
.
Working towards #5298
This PR adds a throttle module that allows execution ingestion engine to load up to the first 500 blocks and start fetching data and executing them even if it's falling far behind.