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.

Sign up for GitHub

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

Jump to bottom

Fix ledger range calculation #217

Merged
merged 77 commits into from
Jul 2, 2024
Merged

Fix ledger range calculation #217

merged 77 commits into from
Jul 2, 2024

Conversation

aditya1702
Copy link
Contributor

@aditya1702 aditya1702 commented Jun 14, 2024 •
edited
Loading

What

Fixes #208 , #210

  • Change GetLedgerRange function to query the meta table in an optimised way. This will be used by the rest of the endpoints to get the oldest and latest ledger details.
  • Add tests for GetLedgerRange

Why

Resolving bugs that were introduced with the new transactions db changes.

Known limitations

N/A

@aditya1702
Add LedgerRangeReader interface
efab5b3
@aditya1702
Add GetLedgerRange implementation for ledgers table
9966f57
@aditya1702
Use GetLedgerRange from the ledgers table for getHealth and getFeeStats
aad48ab
@aditya1702
Change varible name
9e82846
@aditya1702
Add GetLedgerRange method for transactions table
d198238
@aditya1702
Add test for GetLedgerRange for ledgers table
f12d930
@aditya1702
Merge branch 'main' into fix-rpc-bugs
4a2f78c
@aditya1702
Add GetLedgerRange in ConstantLedgerReader
3c9c7a6
@aditya1702
Merge remote-tracking branch 'origin/fix-rpc-bugs' into fix-rpc-bugs
38d6ef5
@aditya1702
Add test for GetLedgerRange in transactions table
fc56898
@aditya1702
Fix nil pointer bugs in getTransactions
a27a176
@aditya1702
Remove latest ledger assertion
ef35cb8
@aditya1702
Comment out the latest ledger assertion
1d9140c
@aditya1702
Remove GetLedgerRange from meta table and use ledgerRangeGetter for g…
1b35c07 
…etHealth and getFeeStats
@aditya1702
Remove ledgerCloseTime
420adc0
@aditya1702
Revert newline change
2343207
@aditya1702
Merge branch 'main' into fix-rpc-bugs
662b2a8
@aditya1702
Revert
1dc4020
@aditya1702
Merge remote-tracking branch 'origin/fix-rpc-bugs' into fix-rpc-bugs
b3cd836
@aditya1702 aditya1702 changed the title [WIP] Fix GetLedgerRange [WIP] Fix ledger range calculation Jun 17, 2024
@aditya1702 aditya1702 changed the title [WIP] Fix ledger range calculation Fix ledger range calculation Jun 17, 2024
@aditya1702 aditya1702 marked this pull request as ready for review June 17, 2024 20:13
@aditya1702 aditya1702 requested a review from psheth9 June 27, 2024 19:28
2opremio
2opremio requested changes Jun 28, 2024
View reviewed changes
Copy link
Contributor

@2opremio 2opremio left a comment

Choose a reason for hiding this comment

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

Almost there! Just a few minor things to go!

@2opremio
Copy link
Contributor

BTW, what's the benchmark result?

@aditya1702
Copy link
Contributor Author

BTW, what's the benchmark result?

@2opremio I pasted the benchmarking here - #217 (comment)

@aditya1702
Copy link
Contributor Author

Benchmarking after changing the query to use sq.Select instead of raw expression:

goos: darwin
goarch: arm64
pkg: github.com/stellar/soroban-rpc/cmd/soroban-rpc/internal/db
BenchmarkGetLedgerRange
BenchmarkGetLedgerRange-12    	   50593	     23656 ns/op

Shaptic
Shaptic reviewed Jul 1, 2024
View reviewed changes
Copy link
Contributor

@Shaptic Shaptic left a comment

Choose a reason for hiding this comment

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

I know this has gone through many many iterations but it seems we've finally landed on one that leads to quite a simple and clean implementation! LGTM from my perspective besides some tiny comments, but I'll wait for the other two to put a formal ✔️ on this one 👏

cmd/soroban-rpc/internal/db/ledger_test.go Outdated Show resolved Hide resolved
cmd/soroban-rpc/internal/db/sqlmigrations/03_ledgers_index.sql Outdated Show resolved Hide resolved
cmd/soroban-rpc/internal/db/ledger.go Show resolved Hide resolved
@aditya1702
Copy link
Contributor Author

aditya1702 commented Jul 1, 2024 •
edited
Loading

@Shaptic Yeah I was also thinking of the same idea and added the ORDER BY ASC clause at the end. This just increases the query time by 0.5 microsecond (which is nothing honestly).

cc @2opremio

tamirms
tamirms approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@tamirms tamirms 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, but I would wait on @2opremio feedback before merging

2opremio
2opremio approved these changes Jul 2, 2024
View reviewed changes
Copy link
Contributor

@2opremio 2opremio left a comment

Choose a reason for hiding this comment

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

Good job! I left a few extra comments, I am approving since I don't want to block the PR anymore, but please try the suggestions I added before merging.

@aditya1702 aditya1702 merged commit 44515d0 into stellar:main Jul 2, 2024
19 checks passed
@2opremio 2opremio mentioned this pull request Jul 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shaptic Shaptic Shaptic left review comments

@tamirms tamirms tamirms approved these changes

@2opremio 2opremio 2opremio approved these changes

@psheth9 psheth9 Awaiting requested review from psheth9

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

transactionHandler's GetLedgerRange is not accurate
5 participants
@aditya1702 @2opremio @Shaptic @tamirms @psheth9