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

ledger-tool: make read_only consistent across bigtable subcommands #34513

Merged
merged 2 commits into from
Dec 18, 2023

Conversation

CriesofCarrots
Copy link
Contributor

Problem

LedgerStorageConfig contains a read_only field, but this isn't being populated correctly for some solana-ledger-tool bigtable subcommands. This could be a footgun for the CLI if someone modified solana-storage-bigtable methods to mutate the bigtable without handling appropriately in solana-ledger-tool.

Summary of Changes

Set more subcommands as read-only

I did confirm the setting works as intended, and prevents a block upload when set. However, the CLI just hangs indefinitely on Preparing the next 1 blocks for upload, which isn't very clear. I'll look at making that error out properly.

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.

I searched the file for:

read_only: false

There are 7 current instances in master.

  • upload needs r/w
  • copy needs r/w for the target db

Of the remaining 5, 4 of them are already changed in this PR. The 5th instance is one I added another comment to, so once we land on the correct setting for that instance, I think we'll have all occurrences accounted for

ledger-tool/src/bigtable.rs Show resolved Hide resolved
@CriesofCarrots
Copy link
Contributor Author

CriesofCarrots commented Dec 18, 2023

  • copy needs r/w for the target db

Incidentally, I was delighted to discover that the two read_only settings in copy() were being handled correctly :)

Copy link

codecov bot commented Dec 18, 2023

Codecov Report

Merging #34513 (1431016) into master (65e10ae) will decrease coverage by 0.1%.
Report is 6 commits behind head on master.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff            @@
##           master   #34513     +/-   ##
=========================================
- Coverage    81.8%    81.8%   -0.1%     
=========================================
  Files         820      820             
  Lines      220869   220869             
=========================================
- Hits       180791   180785      -6     
- Misses      40078    40084      +6     

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.

LGTM

@CriesofCarrots CriesofCarrots merged commit 84a079e into solana-labs:master Dec 18, 2023
34 checks passed
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.

2 participants