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

TransactionScheduler: SchedulerController #33825

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

apfitzge
Copy link
Contributor

Problem

Need control-flow for packet receiving & scheduler.

Summary of Changes

  • Add new struct SchedulerController that is the main interface for the scheduling thread
  • Add basic sanity check tests that loop works. Actual schedule tests are in the scheduler itself, as this class could have drop-in replacements for the scheduler.

Fixes #

@apfitzge
Copy link
Contributor Author

I had forgotten that this PR was necessary before the piping PR. Next PR before the scheduler can be used on master would be piping of CLI & spawning the worker/scheduler threads.

@apfitzge apfitzge marked this pull request as ready for review October 24, 2023 09:56
@codecov
Copy link

codecov bot commented Oct 24, 2023

Codecov Report

Merging #33825 (c165557) into master (b0dcaf2) will increase coverage by 0.0%.
Report is 11 commits behind head on master.
The diff coverage is 97.1%.

@@           Coverage Diff            @@
##           master   #33825    +/-   ##
========================================
  Coverage    81.8%    81.9%            
========================================
  Files         809      810     +1     
  Lines      217643   217961   +318     
========================================
+ Hits       178247   178592   +345     
+ Misses      39396    39369    -27     

Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

looks good, easy to follow. Just few nits

) -> Result<(), SchedulerError> {
match decision {
BufferedPacketsDecision::Consume(_bank_start) => {
let _num_scheduled = self.scheduler.schedule(&mut self.container)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

A slight change of behave here. Currently buffered packets are immediately consumed after decision; here there will be some delay between being scheduled and being picked up by worker thread for processing. Don't know the implication with additional hop, just want to point it out

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think there's a distinction between the two, i.e. scheduled (queued) for consuming vs immediately consuming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I think there's a distinction between the two, i.e. scheduled (queued) for consuming vs immediately consuming.

@apfitzge apfitzge changed the title TransactionScheduelr: SchedulerController TransactionScheduler: SchedulerController Oct 26, 2023
Copy link
Contributor

@tao-stones tao-stones left a comment

Choose a reason for hiding this comment

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

lgtm

@apfitzge apfitzge merged commit ba112a0 into solana-labs:master Oct 27, 2023
32 checks passed
@apfitzge apfitzge deleted the scheduler_controller branch October 27, 2023 01:30
@buffalu
Copy link
Contributor

buffalu commented Oct 27, 2023

noooooo

@buffalu
Copy link
Contributor

buffalu commented Oct 27, 2023

back to rebase hell 😭

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

Successfully merging this pull request may close these issues.

3 participants