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(localstore): remove reserve calculation from localstore #3579

Merged
merged 17 commits into from Nov 24, 2022

Conversation

aloknerurkar
Copy link
Contributor

@aloknerurkar aloknerurkar commented Nov 23, 2022

Checklist

  • I have read the coding guide.
  • My change requires a documentation update, and I have done it.
  • I have added tests to cover my changes.
  • I have filled out the description and linked the related issues.

Description

During the recent storage incentives changes, we found that the reserve size calculation in the localstore was off. A bunch of efforts has already been made to understand and fix this, but due to the dynamic nature of storage depth, it seems like it is not possible to have a static count of reserve size stored in the localstore, which can represent the current reserve size.

The reserve size is now synonymous with the pullIndex as this is the index we use to generate the sample. This index will now be used to represent the reserve chunks in the localstore. So if chunks are put into GC index, which represents the cache, we should remove them from pullIndex as they are no longer part of the reserve. This change will make the addition of pullIndex more strict and only allow syncing related contexts to add the pullIndex.

As the reserve size calculation is now completely removed, there are a lot of simplifications in the mode_put file.

Reserve evictions will now be triggered if we are above reserve capacity as computed with the depth seen by the depthmonitor. This is periodic and will only happen if we are above capacity. Localstore will only trigger GC evictions from now.

All the unit tests are adjusted to now verify the new expected values in the indexes.

A bug related to postageIndex collisions is also fixed. During GC, we were not deleting the postageIndexIndex entry, which led to us searching for a chunk that is not present in the localstore. #3039


This change is Reviewable

@aloknerurkar aloknerurkar marked this pull request as ready for review November 24, 2022 09:07

checkReserveComputation := func(t *testing.T, count uint64) {
t.Helper()
// postage index will not be deleted
Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment needs to be updated

@istae istae linked an issue Nov 24, 2022 that may be closed by this pull request

db.metrics.GCStoreTimeStamps.Set(float64(item.StoreTimestamp))
db.metrics.GCStoreTimeStamps.Set(float64(storedItem.StoreTimestamp))
db.metrics.GCStoreAccessTimeStamps.Set(float64(item.AccessTimestamp))

// delete from retrieve, pull, gc
Copy link
Contributor

Choose a reason for hiding this comment

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

The repetitive error handling can be improved be employing the following:

err = nil
deleteInBatch := func(fn func(*leveldb.Batch, shed.Item) error, batch *leveldb.Batch, keyFields shed.Item) {
	if err != nil {
		return
	}
	err = fn(batch, keyFields)
}

deleteInBatch(db.retrievalDataIndex.DeleteInBatch, batch, item)
deleteInBatch(db.retrievalAccessIndex.DeleteInBatch, batch, item)
deleteInBatch(db.pushIndex.DeleteInBatch, batch, storedItem)
deleteInBatch(db.pullIndex.DeleteInBatch, batch, item)
deleteInBatch(db.gcIndex.DeleteInBatch, batch, item)
deleteInBatch(db.postageIndexIndex.DeleteInBatch, batch, storedItem)
deleteInBatch(db.postageChunksIndex.DeleteInBatch, batch, item)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. This can be done in a lot of places actually. But I dont want to prolong this change as we need to start testing. Will do this in a follow-up PR later.

pkg/localstore/gc.go Outdated Show resolved Hide resolved
pkg/localstore/gc_test.go Outdated Show resolved Hide resolved
pkg/localstore/reserve.go Outdated Show resolved Hide resolved
@istae istae merged commit a276770 into master Nov 24, 2022
@istae istae deleted the localstorefixes.0 branch November 24, 2022 18:57
@aloknerurkar aloknerurkar added this to the 1.10.0 milestone Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Puller rate incorrectly blocks Schelling game
5 participants