core: generic test suite for AncientStore #23707

MichaelMure · 2021-10-11T14:28:56Z

Refactor the freezer test suite into one that can be used for other AncientStore implementation. Ultimately that simplify greatly having an alternative implementation.

To achieve that, a subset of previously private errors have been promoted as part of the AncientStore interface.

Untangle some code in the process: I believe freezer can now be extracted from rawdb if that's
deemed beneficial.

CC @holiman, I believe you are the one to bug about that.

holiman · 2021-10-12T08:33:14Z

I'd like to get #23566 merged before this one.
Aside from that, during the triage , the discussion came up: what is the point of this PR? We're a bit hesitant to commit to a specific API for the ancients. Are you writing an alternative implementation for the ancientstore?

MichaelMure · 2021-10-12T09:03:25Z

core/rawdb/freezer.go

@@ -190,15 +182,15 @@ func (f *freezer) HasAncient(kind string, number uint64) (bool, error) {
 	if table := f.tables[kind]; table != nil {
 		return table.has(number), nil
 	}
-	return false, nil
+	return false, ethdb.ErrAncientUnknownKind


Please notice this change. It looked like a mistake to me so I fixed it.

Actually, looks like this method can be deleted from the codebase now

I removed it in ba1cee6

MichaelMure · 2021-10-12T09:09:12Z

Are you writing an alternative implementation for the ancientstore?

I am, but I don't expect a stable interface. I'm just trying to smooth things from a development perspective:

have a test suite able to validate both the canonical implementation and an alternative
allow to have an alternative implementation returning the same errors in the same conditions (which means having those errors public)
untangle a bit the current code:
- avoid assumptions in the test code about freezer (there was some tiny ones, irrelevant IMHO) and test only the public API
- make the storage concerns (freezer) more orthogonal regarding the higher level concerns (rawdb and up). Or rather the other way around: rawdb should not care about freezer's internals.

MichaelMure · 2021-10-12T09:24:02Z

Speaking of AncientStore's interface, I'd like to bring something to your attention.

One thing I noticed is that it does pretty much force the implementation to have a separate storage for each kind, the same way freezer does. I've been considering storing all the kinds together per block with the assumption that if one is accessed the others are likely to be accessed after that (so some form of prefetching/caching), but that's not something that looks doable reasonably with this interface.

That said, I don't have enough insight at the moment to say if that approach would be valuable so that might be entirely irrelevant.

holiman · 2021-10-12T09:36:24Z

with the assumption that if one is accessed the others are likely to be accessed after that (so some form of prefetching/caching), but that's not something that looks doable reasonably with this interface.

I'm not sure that's a valid assumption. When someone is syncing from you, they might first get header in ranges from you, then spread out the body download among a set of peers, and you'll serve bodies here and there which may or may not line up with what you delivered earlier.

The PR I linked above, however, with the batched reads, is more in line with your proposed schema, however. It gives the reader a consistent interface, meaning that when doing a batched read, the individual fields are guaranteed to be in lock-step with one-another (but not forcing the fields to all be loaded from disk when one is read)

MichaelMure · 2021-10-21T08:57:15Z

Following on this idea, I made two small changes for the KeyValueStore:

return a common error on Get when not found. This allow to distinguish between not found and an actual read error. Also allow for fancy layering on top of a KeyValueStore.
document Delete as idempotent (it's already the case in the current implementations).

holiman · 2021-10-25T18:19:33Z

The #23566 PR is now merged, I guess this will need to be updated?

MichaelMure · 2021-10-26T10:41:38Z

@holiman I rebased and removed HasAncient. It should be good now.

MichaelMure · 2021-10-26T10:52:42Z

Note: it would make sense to extend TestAncientStoreSuite for the new API introduced in #23566.

MichaelMure · 2021-10-26T14:52:44Z

I completed the test suite to cover those. I'm really done this time! (maybe?)

MichaelMure · 2021-11-10T14:57:28Z

@holiman would it be time to merge this?

holiman · 2021-11-10T15:30:19Z

We discussed this yesteday on our PR triage, and there remains some hesitancy about this, again. Same as I wrote earlier:

Aside from that, during the triage , the discussion came up: what is the point of this PR? We're a bit hesitant to commit to a specific API for the ancients.

Merging this change makes it harder for us to change the API, and we're not sure that we've arrived at the Perfect API yet. Things may (and do) still change, and having to worry about the third-part impact another public API is cumbersome.

MichaelMure · 2021-11-10T15:45:46Z

I promise I really don't expect this API to be stable. As a maintainer myself I know it's uncomfortable to expose internals but that's not what I'm trying to do here.

I'm merely trying to make this API easier to use, maintain and build on. This PR document some existing behavior, allow to distinguish some failure mode (not found vs actual error) and generalize/clean the test suite.

I believe those changes are useful, regardless if I'm using the API as a third party or not.

MichaelMure · 2021-11-10T17:39:15Z

If that helps, those interfaces could be marked as internal/unstable.

egalano · 2021-12-01T17:50:14Z

Hi @holiman. What would it take to get this PR accepted? Michael is on the Infura team and as he mentioned, we're working on an alternative ancient store experiment that we'd like to share if the experiment works out after load testing. We are fully content to accept that we are building on an internal API which will change in the future and we don't expect this to be maintained as a public API. If there are additional changes you'd like us to make to this PR before it can be merged please let us know.

holiman · 2021-12-02T14:02:19Z

@egalano we'll discuss it internally again next triage

holiman · 2021-12-07T09:32:42Z

Triage discussion: we'll do another round of review on this PR

karalabe · 2022-01-11T09:41:38Z

We've discussed to get this PR in, but we need to finish and merge a prerequisite first (#23954) as that also changes the code and interfaces a bit.

holiman · 2022-01-11T09:42:07Z

We're tentatively agreeing that this PR could be an ok addition, but as said earlier: the freezer API is still not stable. So this PR has bitrotted a bit and needs a rebase, and further, we have more changes in the pipeline, such as #23954

MichaelMure · 2022-01-26T11:33:28Z

@holiman should I rebase this PR before or after #23954 get merged?

MichaelMure · 2022-03-14T18:46:06Z

@holiman I rebased and updated my PR now that #23954 is merged. Can we move towards merging and avoid bitrot?

rjl493456442

Left a few comments.

ethdb/database.go

core/rawdb/freezer_batch.go

core/rawdb/freezer_table_test.go

rjl493456442 · 2022-03-15T08:36:36Z

ethdb/database.go

@@ -129,9 +154,11 @@ type AncientWriter interface {
 // AncientWriteOp is given to the function argument of ModifyAncients.
 type AncientWriteOp interface {
 	// Append adds an RLP-encoded item.
+	// Returns ErrAncientOutOrderInsertion if attempting to insert items out of order.


I think this kind should also be checked and return the ErrAncientUnknownKind if necessary?

Hmmm. freezerBatch currently panic if an unknown kind is used. While that would make sense to return ErrAncientUnknownKind, it also would be a programming error to provide an unknown kind rather than a runtime problem, so a panic seems more appropriate.
Which one do you prefer?

I think it's fine to panic, if that's what we already do. Writing wrong kind is definitely a huge programming error.

It's ready to merge then.

ethdb/database.go

ethdb/leveldb/leveldb.go

ethdb/snapshot.go

rjl493456442 · 2022-03-16T03:01:21Z

btw, I just want you know that we may change the database interface again which introduces the ancient store routing. #24481 My PR is not reviewed right now, but it's kind of necessary.

Refactor the freezer test suite into one that can be used for other AncientStore implementation. To achieve that, a subset of previously private errors have been promoted as part of the AncientStore interface. Untangle some code in the process: I believe freezer can now be extracted from rawdb if that's deemed beneficial.

This allow to distinguish between not found and an actual read error. Also allow for fancy layering on top of a KeyValueStore. Also, document KeyValueWriter.Delete as idempotent.

MichaelMure · 2022-03-17T13:24:30Z

I think it's good to go now. I can still adjust #23707 (comment) if I get an answer.

btw, I just want you know that we may change the database interface again which introduces the ancient store routing.

That's completely fine. Again, I'm not expecting to have a stable interface, I'm just trying to make things easier to work with for all of us: well defined interfaces and reusable test suite.

MichaelMure requested review from holiman, karalabe and rjl493456442 as code owners October 11, 2021 14:28

ligi added the status:triage label Oct 12, 2021

MichaelMure commented Oct 12, 2021

View reviewed changes

ligi removed the status:triage label Oct 13, 2021

MichaelMure force-pushed the ancient-test branch from 97db15f to 0665d4f Compare October 21, 2021 08:54

MichaelMure force-pushed the ancient-test branch from 0665d4f to ded6810 Compare October 26, 2021 10:19

MichaelMure force-pushed the ancient-test branch from ba1cee6 to 1e30457 Compare October 26, 2021 12:40

holiman added the status:triage label Dec 2, 2021

karalabe removed the status:triage label Jan 11, 2022

MichaelMure force-pushed the ancient-test branch from d18d329 to 6323523 Compare March 14, 2022 17:56

rjl493456442 reviewed Mar 15, 2022

View reviewed changes

MichaelMure added 6 commits March 17, 2022 14:04

core: return a common error on KeyValueReader.Get when not found

8a2fb4b

This allow to distinguish between not found and an actual read error. Also allow for fancy layering on top of a KeyValueStore. Also, document KeyValueWriter.Delete as idempotent.

ethdb: remove unused HasAncient from AncientReader

25b1b05

ethdb: complete the test suite for AncientRange and ReadAncients

b7b440d

ethdb: document and return public errors in Snapshot

93bb149

ethdb: merge different errors into a single ErrNotFound error

a9ac20d

MichaelMure force-pushed the ancient-test branch from 6323523 to a9ac20d Compare March 17, 2022 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: generic test suite for AncientStore #23707

core: generic test suite for AncientStore #23707

MichaelMure commented Oct 11, 2021

holiman commented Oct 12, 2021

MichaelMure Oct 12, 2021

holiman Oct 25, 2021

MichaelMure Oct 26, 2021

MichaelMure commented Oct 12, 2021

MichaelMure commented Oct 12, 2021

holiman commented Oct 12, 2021

MichaelMure commented Oct 21, 2021

holiman commented Oct 25, 2021

MichaelMure commented Oct 26, 2021

MichaelMure commented Oct 26, 2021

MichaelMure commented Oct 26, 2021

MichaelMure commented Nov 10, 2021

holiman commented Nov 10, 2021

MichaelMure commented Nov 10, 2021

MichaelMure commented Nov 10, 2021

egalano commented Dec 1, 2021 •

edited

holiman commented Dec 2, 2021

holiman commented Dec 7, 2021

karalabe commented Jan 11, 2022

holiman commented Jan 11, 2022

MichaelMure commented Jan 26, 2022

MichaelMure commented Mar 14, 2022

rjl493456442 left a comment

rjl493456442 Mar 15, 2022

MichaelMure Mar 15, 2022

holiman Mar 17, 2022

MichaelMure Mar 17, 2022

rjl493456442 commented Mar 16, 2022

MichaelMure commented Mar 17, 2022

core: generic test suite for AncientStore #23707

Are you sure you want to change the base?

core: generic test suite for AncientStore #23707

Conversation

MichaelMure commented Oct 11, 2021

holiman commented Oct 12, 2021

MichaelMure Oct 12, 2021

Choose a reason for hiding this comment

holiman Oct 25, 2021

Choose a reason for hiding this comment

MichaelMure Oct 26, 2021

Choose a reason for hiding this comment

MichaelMure commented Oct 12, 2021

MichaelMure commented Oct 12, 2021

holiman commented Oct 12, 2021

MichaelMure commented Oct 21, 2021

holiman commented Oct 25, 2021

MichaelMure commented Oct 26, 2021

MichaelMure commented Oct 26, 2021

MichaelMure commented Oct 26, 2021

MichaelMure commented Nov 10, 2021

holiman commented Nov 10, 2021

MichaelMure commented Nov 10, 2021

MichaelMure commented Nov 10, 2021

egalano commented Dec 1, 2021 • edited

holiman commented Dec 2, 2021

holiman commented Dec 7, 2021

karalabe commented Jan 11, 2022

holiman commented Jan 11, 2022

MichaelMure commented Jan 26, 2022

MichaelMure commented Mar 14, 2022

rjl493456442 left a comment

Choose a reason for hiding this comment

rjl493456442 Mar 15, 2022

Choose a reason for hiding this comment

MichaelMure Mar 15, 2022

Choose a reason for hiding this comment

holiman Mar 17, 2022

Choose a reason for hiding this comment

MichaelMure Mar 17, 2022

Choose a reason for hiding this comment

rjl493456442 commented Mar 16, 2022

MichaelMure commented Mar 17, 2022

egalano commented Dec 1, 2021 •

edited