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

History Archive Support for State Archival #4610

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

SirTyson
Copy link
Contributor

@SirTyson SirTyson commented Jan 8, 2025

Description

Resolves #4593

This adds support for the Hot Archive BucketList in History Archives. This includes changes to publish, catchup, and assume state.

TODO: More robust p23 upgrade testing, specifically around upgrades that occur on or during checkpoint boundaries.

Upgrade tests have been added, so unit tests should have complete coverage now.

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

@SirTyson SirTyson force-pushed the hot-archive-history branch 2 times, most recently from c6d900c to 6ffde65 Compare January 15, 2025 19:25
@SirTyson SirTyson marked this pull request as ready for review January 15, 2025 19:26
@SirTyson SirTyson force-pushed the hot-archive-history branch from 6ffde65 to 39e74f8 Compare January 15, 2025 20:18
@SirTyson SirTyson requested review from dmkozh and graydon January 15, 2025 20:18
@SirTyson SirTyson force-pushed the hot-archive-history branch 5 times, most recently from 2f0f86a to e258f51 Compare January 22, 2025 02:20
@SirTyson SirTyson force-pushed the hot-archive-history branch from e258f51 to 69ca558 Compare February 18, 2025 19:20
graydon
graydon previously approved these changes Feb 26, 2025
Copy link
Contributor

@graydon graydon 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! Handful of nits but you can skip 'em / do in followups at your leisure.

@@ -75,17 +82,37 @@ struct HistoryArchiveState
static constexpr size_t MAX_HISTORY_ARCHIVE_BUCKET_SIZE =
1024ull * 1024ull * 1024ull * 100ull; // 100 GB

static unsigned const HISTORY_ARCHIVE_STATE_VERSION;
static inline unsigned const HISTORY_ARCHIVE_STATE_VERSION_PRE_PROTOCOL_22 =
Copy link
Contributor

Choose a reason for hiding this comment

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

These names are a little odd. You have pre-22 and post-22 but no 22. Also I think 23 is the boundary? Maybe rename as ..VERSION_BEFORE_HOT_ARCHIVE and ...VERSION_WITH_HOT_ARCHIVE ?

hash = mLiveBucketList->getHash();
SHA256 hsh;
hsh.add(mLiveBucketList->getHash());
hsh.add(mHotArchiveBucketList->getHash());
Copy link
Contributor

Choose a reason for hiding this comment

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

It lives!

@@ -1094,15 +1112,17 @@ BucketManager::startBackgroundEvictionScan(uint32_t ledgerSeq,
mSnapshotManager->copySearchableLiveBucketListSnapshot();
auto const& sas = cfg.stateArchivalSettings();

using task_t = std::packaged_task<EvictionResultCandidates()>;
using task_t =
std::packaged_task<std::unique_ptr<EvictionResultCandidates>()>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious: why the extra layer of indirection here with the unique_ptr?

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 threw a bunch of consts in EvictionResultCandidates, which removed the default copy constructor and copy assignment operator, which I need in BucketManager.cpp::1154. Eviction candidates can be pretty large, so we probably don't want to copy them anyway. I think the correct solution is to make EvictionResultCandidates and use unique_ptr everywhere.

@@ -1250,51 +1270,71 @@ BucketManager::assumeState(HistoryArchiveState const& has,
releaseAssert(threadIsMain());
releaseAssertOrThrow(mConfig.MODE_ENABLES_BUCKETLIST);

// TODO: Assume archival bucket state
// Dependency: HAS supports Hot Archive BucketList
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this line of comment is residual from the removed TODO, and should also go?

auto b = getBucketByHashInternal(h, mSharedLiveBuckets);
if (!b)
// Returns filename, hash, and whether it's a hot archive bucket
auto loadFilenameAndHash =
Copy link
Contributor

Choose a reason for hiding this comment

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

Why make a lambda just to call it? There's only one caller.

@SirTyson SirTyson force-pushed the hot-archive-history branch from c6aaaf2 to 2504748 Compare March 2, 2025 01:36
@SirTyson SirTyson force-pushed the hot-archive-history branch from 2504748 to cafb222 Compare March 2, 2025 19:43
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.

Add History Archive support for Hot Archive Bucket
2 participants