Add integrate mix-in #61
Conversation
storage/integrate.go
Outdated
| return newTiles | ||
| } | ||
|
|
||
| // fullTile represents a "fully populated" tile, i.e. it has all non-ephemeral internaly nodes |
There was a problem hiding this comment.
| // fullTile represents a "fully populated" tile, i.e. it has all non-ephemeral internaly nodes | |
| // fullTile represents a "fully populated" tile, i.e. it has all non-ephemeral internal nodes |
storage/integrate.go
Outdated
| r.Lock() | ||
| defer r.Unlock() | ||
| k := layout.TilePath(uint64(tileID.Level), tileID.Index, treeSize) | ||
| if e, ok := r.entries[k]; ok && !e.Equals(t) { |
There was a problem hiding this comment.
You should be able to drop this equality check if the read-once semantics for any given tile are fixed.
There was a problem hiding this comment.
That's gone away now
storage/integrate.go
Outdated
| return errors.Join(tc.err...) | ||
| } | ||
|
|
||
| // Visit should be called once for each newly set non-ephemeral node in the |
There was a problem hiding this comment.
nit: This comment should start with Visitor .... It appears to jump over what this function does and talks about how its return value should be used.
storage/integrate.go
Outdated
| // | ||
| // Note that by itself, this cache does not update any persisted state. | ||
| type tileWriteCache struct { | ||
| sync.Mutex |
There was a problem hiding this comment.
I don't see where this is used in parallel. Can we remove this mutex to keep it simpler looking?
There was a problem hiding this comment.
Yeah, it was there from an abundance of caution, but it should never be used in parallel since it's driven by the visitor.
Removed.
storage/integrate.go
Outdated
| if err := tc.Err(); err != nil { | ||
| return 0, nil, nil, err | ||
| } |
There was a problem hiding this comment.
Worth moving this up a few lines? Maybe even checking it before and after baseRange.AppendRange(newRange, visitor) ? Reasoning being that any errors in here are likely to cause failures in the other operations that depend on the writeTileCache succeeding. It's better to return the root error rather than a symptomatic error.
storage/integrate.go
Outdated
| // | ||
| // The getTile param must know how to fetch the specified tile from storage. It must return an instance of os.ErrNotExist | ||
| // (either directly, or wrapped) if the requested tile was not found. | ||
| func NewTreeBuilder(getTile func(ctx context.Context, tileID TileID, treeSize uint64) (*api.HashTile, error)) *TreeBuilder { |
There was a problem hiding this comment.
As discussed in person, making this function take all the tiles to look up in batch allows for more flexibility in implementation. And as a side effect, makes it so that we don't end up fetching the same tile multiple times if there are several nodes required from the same tile.
There was a problem hiding this comment.
This is a great observation, thank you!
I've re-worked quite a bit of how this hangs together to support this, PTAL.
This PR adds a storage mix-in for performing the integration of new entries into the log.
Towards #6 & #23