[TieredStorage] Enable hot-storage in TieredStorage::write_accounts()

[yhchiang-sol]

Problem

While the implementation of hot-storage reader and writer
are mostly done, it is not yet connected to TieredStorage.

Summary of Changes

This PR enables hot-storage in TieredStorage::write_accounts().

Test Plan

Completes the existing tests in TieredStorage to directly
write and read from a TieredStorage with the hot storage format.

[yhchiang-sol]

So in the upper layer, AccountsFile::append_accounts returns an Option instead. This means when we have an error here, it's either panic or we hide the error.

I can probably improve the comment a bit by saying that it is guaranteed to have no reader here as we just checked its is_read_only() so we expect reader.set() to be successful.

[yhchiang-sol]

I will create a separate PR that put all the test utils into one rs file.

[yhchiang-sol]

Test utils are moved to test_utils.rs in #35065.

[brooksprumo]

Why create a reader here at all? What about deferring creating the mmap until something reads from the tiered storage?

[yhchiang-sol]

Good idea. I was under the impression that it will be used right away, but I think having things in lazy style works better.

[yhchiang-sol]

Put it in a separate PR (#35063) as it needs to create another field as well as potentially change the return value format of the reader() API.

[brooksprumo]

I was under the impression that it will be used right away

We'd have to check the flush and clean code. Maybe immediately after flush we clean that slot?

[yhchiang-sol]

Flush typically only means finishing writing. So it's up to us to determine which way works the best.

In Rocks, I remember it has an option to determine whether to keep entries in memory after flushing from memtable. (i.e., the entries were available in mem-table, but after flushing to disk as a sst file, it no longer be available in memory). This depends on whether our workload is more likely to read recent data.

[brooksprumo]

The flush and clean I'm referring to are the the specific accounts-db functions/processes that occur, not something else like file i/o.

Once a slot is rooted, the contents for that slot (and any older) are written to disk from the accounts write cache. This process is called flush. We may mark this slot as a candidate for clean immediately. In that case, we'll look at this slot shortly.

clean is the process that identifies old accounts that have been rendered unnecessary by newer writes/updates to the same account. This process updates the accounts index, and also updates the append vecs for their alive bytes (which is then used by shrink).

[brooksprumo]

For the sake of this and subsequent PRs, I think we can use the current implementation and then change/improve it over time/usage. I.e. we don't need to wait for #35063 to land.

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (dd30175) 81.6% compared to head (bbfff2a) 81.6%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #35049     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         830      830             
  Lines      224946   224955      +9     
=========================================
+ Hits       183674   183678      +4     
- Misses      41272    41277      +5     

[brooksprumo]

lgtm

As I said in #35063 (review), is write_accounts() thread safe? What happens if different threads call this method on the same instance?

If that's programmatically possible, then we should fix it. And test it too. (Please do this in a separate PR though 😸)

[brooksprumo]

For the sake of this and subsequent PRs, I think we can use the current implementation and then change/improve it over time/usage. I.e. we don't need to wait for #35063 to land.

[yhchiang-sol]

As I said in #35063 (review), is write_accounts() thread safe? What happens if different threads call this method on the same instance?
If that's programmatically possible, then we should fix it. And test it too. (Please do this in a separate PR though 😸)

As each TieredStorage instance represents one accounts file and its write_accounts is designed to only invoke once, it is not thread-safe (and I think accounts-db does not invoke append_accounts() more than once on the same file simultaneously, it will check whether it has spare space after the first one finishes). Will create a separate PR to add comments about its thread-safety.