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

Convert live entries to init entries in lowest level of bucketlist #4492

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ThomasBrady
Copy link
Contributor

@ThomasBrady ThomasBrady commented Oct 1, 2024

Description

Resolves #193

Changes BucketOutputIterator::put to convert LIVEENTRY to INITENTRY in the lowest level of LiveBuckets.

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

@ThomasBrady ThomasBrady requested review from SirTyson and removed request for SirTyson October 1, 2024 20:56
@ThomasBrady ThomasBrady changed the title Convert live entries to init entries in lowest level of bucketlist WIP: Convert live entries to init entries in lowest level of bucketlist Oct 1, 2024
@ThomasBrady ThomasBrady changed the title WIP: Convert live entries to init entries in lowest level of bucketlist Convert live entries to init entries in lowest level of bucketlist Oct 2, 2024
@ThomasBrady ThomasBrady requested a review from SirTyson October 2, 2024 17:36
@ThomasBrady ThomasBrady force-pushed the lowest-level-it-output branch from 3dc4da6 to 3956cd5 Compare November 22, 2024 19:20
@ThomasBrady
Copy link
Contributor Author

@SirTyson can you PTAL?

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.

Overall LGTM. Just one logic cleanup and I think a higher level test would be more helpful here vs. a low level one.

src/bucket/BucketOutputIterator.cpp Outdated Show resolved Hide resolved
src/bucket/test/BucketTests.cpp Outdated Show resolved Hide resolved
@ThomasBrady ThomasBrady force-pushed the lowest-level-it-output branch from a03be99 to 9a6176c Compare November 25, 2024 19:40
src/bucket/Bucket.h Outdated Show resolved Hide resolved
@ThomasBrady ThomasBrady force-pushed the lowest-level-it-output branch from 9a6176c to 5d69fa4 Compare November 25, 2024 19:55
@ThomasBrady ThomasBrady requested a review from SirTyson November 25, 2024 19:58
@ThomasBrady ThomasBrady force-pushed the lowest-level-it-output branch from 4fbd14d to 0e8f83b Compare November 26, 2024 20:00
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.

Sorry, I think I was a little unclear in my last comment. A little cleanup on the interface, and I think the test case still needs improving.

src/bucket/BucketBase.cpp Outdated Show resolved Hide resolved
src/bucket/BucketBase.h Outdated Show resolved Hide resolved
src/bucket/BucketOutputIterator.cpp Show resolved Hide resolved
@@ -521,6 +522,22 @@ TEST_CASE_VERSIONS("live bucket tombstones expire at bottom level",
REQUIRE(e0.nDead != 0);
REQUIRE(e1.nDead != 0);
REQUIRE(e2.nDead == 0);
if (protocolVersionStartsFrom(
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't actually test our fix. The problem is, in old bucketlist code that has long since been fixed, live entries where written to the bottom level. The current code doesn't exhibit this behavior. What you're testing here is if the current BucketList logic works, but not that your conversion code from the buggy BucketList -> good bucketlist actually works. What we should do is populate a BucketList, but specifically populate it with buggy contents such that the bottom level has live entries (see "live bucket tombstones expire at bottom level" for populating a bucketList without addBatch). Then, add empty batched to your BucketList until all entries have coallesced in the bottom bucket (see "hot archive bucket tombstones expire at bottom level" for this example). Finally, scan the bottom bucket and make sure they're all init.

@ThomasBrady ThomasBrady force-pushed the lowest-level-it-output branch 2 times, most recently from df3dc32 to 70bce90 Compare November 27, 2024 01:49
@ThomasBrady ThomasBrady requested a review from SirTyson November 27, 2024 01:50
@ThomasBrady ThomasBrady force-pushed the lowest-level-it-output branch 2 times, most recently from 0276edd to cacef0b Compare November 27, 2024 05:51
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.

Test looks great! One issue that I didn't notice (until your test case, which was super helpful), we need to gate this feature on protocol 23. Currently, we gate on FIRST_PROTOCOL_SUPPORTING_INITENTRY_AND_METAENTRY, which is before p23. This means catchup will fail, as we'll change the bucket hashes for p22 for example. We need to add a new constexpr, like FIRST_PROTOCOL_CONVERTING_BOTTOM_LEVEL_LIVE_TO_INIT or something like that which is equal to 23. Sorry I didn't catch this before!

src/bucket/BucketOutputIterator.cpp Show resolved Hide resolved
@ThomasBrady ThomasBrady force-pushed the lowest-level-it-output branch from cacef0b to 994087f Compare November 27, 2024 18:42
@ThomasBrady ThomasBrady requested a review from SirTyson November 27, 2024 18:42
@SirTyson SirTyson enabled auto-merge December 2, 2024 15:56
auto-merge was automatically disabled December 2, 2024 22:48

Head branch was pushed to by a user without write access

@ThomasBrady ThomasBrady force-pushed the lowest-level-it-output branch from 994087f to 85381f5 Compare December 2, 2024 22:48
@ThomasBrady
Copy link
Contributor Author

@SirTyson I had to rebase, can you re-enable automerge?

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