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

BigtableUploadService: increment start_slot to prevent rechecks #33870

Merged
merged 1 commit into from
Oct 26, 2023

Conversation

CriesofCarrots
Copy link
Contributor

Problem

Both solana-ledger-tool bigtable upload and the BigtableUploadService call solana_ledger::bigtable_upload::upload_confirmed_blocks() in a loop. However, they treat the return value (last slot checked) slightly differently.
Ledger tool increments the value to start checking on a fresh slot:

starting_slot = last_slot_checked.saturating_add(1);

The BigtableUploadService does not:

Ok(last_slot_uploaded) => start_slot = last_slot_uploaded,

This means that the BigtablUploadService is rechecking the starting slot of a range on every iteration of the loop. It's probably not a lot of wasted work, but not nothing.

I don't think there is a specific reason the BigtableUploadService doesn't increment; I think I probably just missed it when putting together #26030

Summary of Changes

Add increment

Incidentally, this change should be enough to fix the infinite loop in #33831, but I still want to land the fix in #33861.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #33870 (117f71f) into master (70107e2) will decrease coverage by 0.1%.
The diff coverage is 0.0%.

@@            Coverage Diff            @@
##           master   #33870     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         809      809             
  Lines      217824   217824             
=========================================
- Hits       178387   178327     -60     
- Misses      39437    39497     +60     

Copy link
Contributor

@ilya-bobyr ilya-bobyr left a comment

Choose a reason for hiding this comment

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

I was also following this issue.
The change looks good to me.

Copy link
Contributor

@steviez steviez 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 to me and makes sense given that upload_confirmed_blocks() is inclusive of ending_slot:

let blockstore_slots: Vec<_> = blockstore
.rooted_slot_iterator(starting_slot)
.map_err(|err| {
format!("Failed to load entries starting from slot {starting_slot}: {err:?}")
})?
.take_while(|slot| *slot <= ending_slot)
.collect();

@CriesofCarrots CriesofCarrots merged commit 22503f0 into solana-labs:master Oct 26, 2023
32 checks passed
@CriesofCarrots CriesofCarrots added the v1.17 PRs that should be backported to v1.17 label Oct 26, 2023
mergify bot pushed a commit that referenced this pull request Oct 26, 2023
CriesofCarrots added a commit that referenced this pull request Oct 26, 2023
…s (backport of #33870) (#33886)

BigtableUploadService: increment start_slot to prevent rechecks (#33870)

Increment start_slot

(cherry picked from commit 22503f0)

Co-authored-by: Tyera <tyera@solana.com>
anwayde pushed a commit to firedancer-io/solana that referenced this pull request Nov 16, 2023
…s (backport of solana-labs#33870) (solana-labs#33886)

BigtableUploadService: increment start_slot to prevent rechecks (solana-labs#33870)

Increment start_slot

(cherry picked from commit 22503f0)

Co-authored-by: Tyera <tyera@solana.com>
anwayde pushed a commit to firedancer-io/solana that referenced this pull request Nov 16, 2023
…s (backport of solana-labs#33870) (solana-labs#33886)

BigtableUploadService: increment start_slot to prevent rechecks (solana-labs#33870)

Increment start_slot

(cherry picked from commit 22503f0)

Co-authored-by: Tyera <tyera@solana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v1.17 PRs that should be backported to v1.17
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants