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

soroban-rpc: Ingest temp ledger entry evictions #926

Merged
merged 7 commits into from
Sep 5, 2023

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Sep 5, 2023

What

In stellar/go#5037 temporary ledger entries which are evicted are excluded from the change reader. So, we need to handle the deletions explicitly by the EvictedTemporaryLedgerKeys() method on the LedgerCloseMeta struct.

Known limitations

We aren't able to test eviction of temporary ledger keys through an integration test because stellar-core needs to implement a testing flag to allow evictions to occur more quickly. So instead I have implemented a unit test where I have hard coded a ledger which contains temporary ledger entry evictions.

@2opremio
Copy link
Contributor

2opremio commented Sep 5, 2023

I am confused. Doesn’t this duplicate #925 ?

@2opremio
Copy link
Contributor

2opremio commented Sep 5, 2023

Also, I am missing a test.

@tamirms
Copy link
Contributor Author

tamirms commented Sep 5, 2023

I am confused. Doesn’t this duplicate #925 ?

yes, I worked on this during the morning as a follow up to stellar/go#5037 while not realizing that you were working on the same thing in #925 .

Also, I am missing a test.

I will probably take the test from #925 😄

@tamirms tamirms requested a review from a team September 5, 2023 19:36
@tamirms tamirms marked this pull request as ready for review September 5, 2023 19:36
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

overall looks good. I did had quite many comments, but all of these were non-blocking.

cmd/soroban-rpc/internal/ingest/service.go Outdated Show resolved Hide resolved
@tamirms tamirms enabled auto-merge (squash) September 5, 2023 20:50
@tamirms tamirms merged commit 9615897 into main Sep 5, 2023
19 of 20 checks passed
@tamirms tamirms deleted the temp-ledger-entry-evictions branch September 5, 2023 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants