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

Fix meta #3844

Closed
wants to merge 4 commits into from
Closed

Fix meta #3844

wants to merge 4 commits into from

Conversation

sisuresh
Copy link
Contributor

Description

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

virtual void erase(InternalLedgerKey const& key) = 0;
virtual LedgerTxnEntry load(InternalLedgerKey const& key) = 0;
virtual LedgerTxnEntry load(InternalLedgerKey const& key,
bool loadExpiredEntry = false) = 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I made this an optional parameter in the interest of time. We're going to remove this anyways so it should be fine.

@sisuresh sisuresh marked this pull request as draft July 20, 2023 22:02
@sisuresh
Copy link
Contributor Author

Marked as draft so we don't merge this in prior to when the 19.13.0 release is cut.

@@ -1526,15 +1461,17 @@ LedgerTxn::Impl::getNewestVersion(InternalLedgerKey const& key,

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to add an expiration check above in LedgerTxn::Impl::getNewestVersion now that mEntry may contain expired entries since load may make an expired entry active. This also applies in getNewestVersionEntryMap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. Fixed

Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

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

LGTM after the src/transactions/RestoreFootprintOpFrame.cpp issue with last modified ledgerSeq is fixed. We're not gonna keep this so you can just loadWithoutRecord, then if you actually update something just load another copy.

Copy link
Contributor

@SirTyson SirTyson left a comment

Choose a reason for hiding this comment

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

LGTM

@sisuresh
Copy link
Contributor Author

Resolved by #3857

@sisuresh sisuresh closed this Jul 31, 2023
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