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

send-transactions: optimize retry pool #31

Merged

Conversation

fanatid
Copy link

@fanatid fanatid commented Mar 3, 2024

Moved from solana-labs#35081

Problem

Transactions not included in the retry pool on full utilization

Summary of Changes

  • do not insert transactions with zero max_retries to the retry pool
  • remove transactions reached max_retries in the same iteration of the loop
  • dynamically select sleep time between iterations based on last_sent_time in TransactionInfo

@mergify mergify bot requested a review from a team March 3, 2024 17:47
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Mar 13, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 13, 2024
@CriesofCarrots
Copy link

Looks like this one needs a rebase to fixup some whitespace lint.

@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Mar 13, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 13, 2024
@CriesofCarrots CriesofCarrots added the CI Pull Request is ready to enter CI label Mar 15, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Mar 15, 2024
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.9%. Comparing base (151675b) to head (1e6a6fc).
Report is 18 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##           master      #31     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         836      836             
  Lines      226629   226634      +5     
=========================================
+ Hits       185688   185690      +2     
- Misses      40941    40944      +3     

@CriesofCarrots CriesofCarrots requested a review from t-nelson March 25, 2024 21:38
Copy link

@godmodegalactus godmodegalactus left a comment

Choose a reason for hiding this comment

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

LGTM

@KirillLykov
Copy link

generally good changes, i don't understand some minor details (see questions in the comments)

@fanatid fanatid force-pushed the send-transactions-opt-retry-master-based branch from 1e6a6fc to c7c05af Compare December 2, 2024 20:47
@fanatid
Copy link
Author

fanatid commented Dec 2, 2024

updated with fixes

@@ -821,30 +872,11 @@ mod test {
config.batch_size,
&stats,
);
assert_eq!(transactions.len(), 1);
Copy link

@KirillLykov KirillLykov Dec 5, 2024

Choose a reason for hiding this comment

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

@fanatid why we don't need this code any longer?

Copy link
Author

Choose a reason for hiding this comment

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

because we remove both transactions from hashmap in one process_transactions call

@KirillLykov
Copy link

@fanatid please update the branch

@KirillLykov KirillLykov merged commit 0abfe27 into anza-xyz:master Dec 6, 2024
40 checks passed
@fanatid fanatid deleted the send-transactions-opt-retry-master-based branch December 6, 2024 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants