[no-release-notes] go: nbs,chunks: Have AddTableFilesToManifest check all dependencies are present. #8824
Conversation
… all dependencies are present.
| @@ -196,7 +195,7 @@ func trueUpBackingManifest(ctx context.Context, root hash.Hash, backing *journal | |||
| if err != nil { | |||
| return manifestContents{}, err | |||
| } else if !ok { | |||
| return manifestContents{}, fmt.Errorf("manifest not found when opening chunk journal") | |||
| return manifestContents{}, nil | |||
There was a problem hiding this comment.
We changed the semantics here so that we can open a journal file without a manifest already existing.
We do this as part of Clone in some cases, where we copy the journal from a remote into our local store, and then we AddTableFilesToManifest to add it to our manifest. The new sanity checking logic wants to open the journal and read its chunks before we agree to add it to the store. This change allows us to do that.
The change below to ParseIfExists stitches it together, by continuing to go to the backing manifest as long as we have never loaded an actual manifest here.
There was a problem hiding this comment.
Probably worth a comment to that effect
| @@ -654,6 +654,18 @@ func (rs *RemoteChunkStore) AddTableFiles(ctx context.Context, req *remotesapi.A | |||
| return &remotesapi.AddTableFilesResponse{Success: true}, nil | |||
| } | |||
|
|
|||
| func (rs *RemoteChunkStore) getAddrs(version string) chunks.GetAddrsCurry { | |||
| getAddrs := func(c chunks.Chunk) chunks.GetAddrsCb { | ||
| return func(ctx context.Context, ins hash.HashSet, filter chunks.PendingRefExists) error { | ||
| return walkAddrs(c, func(h hash.Hash, _ bool) error { | ||
| if !filter(h) { |
There was a problem hiding this comment.
Probably too difficult to change but this is the opposite of the usual filter idiom
| @@ -196,7 +195,7 @@ func trueUpBackingManifest(ctx context.Context, root hash.Hash, backing *journal | |||
| if err != nil { | |||
| return manifestContents{}, err | |||
| } else if !ok { | |||
| return manifestContents{}, fmt.Errorf("manifest not found when opening chunk journal") | |||
| return manifestContents{}, nil | |||
There was a problem hiding this comment.
Probably worth a comment to that effect
| } | ||
|
|
||
| func (nbs *NomsBlockStore) updateManifestAddFiles(ctx context.Context, updates map[hash.Hash]uint32, appendixOption *ManifestAppendixOption) (mi ManifestInfo, err error) { | ||
| func (nbs *NomsBlockStore) updateManifestAddFiles(ctx context.Context, updates map[hash.Hash]uint32, appendixOption *ManifestAppendixOption, gcGen *hash.Hash, sources chunkSourceSet) (mi ManifestInfo, gcGenDifferent bool, err error) { |
There was a problem hiding this comment.
Is it worth defining a type alias for gcGen? Probably useful to distinguish from other random hashes
| } | ||
| eg, ctx := errgroup.WithContext(ctx) | ||
| var mu sync.Mutex | ||
| for h, c := range files { |
There was a problem hiding this comment.
I'm unfamiliar with the semantics here so it's not obvious what value c represents or how it feeds into the Open call below
There was a problem hiding this comment.
Looks good, I don't fully understand the different clone paths and the process for incorporating a server's journal file into another's. Like does this just prevent cloning a corrupt journal file? Also do we just dump the journal file into noms/ to access in the future or do we reformat it?
| } | ||
| } | ||
|
|
||
| // For each chunk in sources, walk all its addresses with |getAddrs| and ensure that none are missing from a call to |refCheck|. |
There was a problem hiding this comment.
so basically, the chunkstores are self-consistent and do not include any intermediate nodes with missing refs?
| } | ||
| // If these sources get added to the store, they will get cloned. | ||
| // Either way, we want to close these instances when we are done. | ||
| defer chunkSources.close() |
There was a problem hiding this comment.
does the newest go version do this correctly? or does it just try to close the last very n-times?
| // store and the store was in a consistent state when | ||
| // we added them. | ||
| // | ||
| // We currently do not take any chunk dependencies |
There was a problem hiding this comment.
GC is irrelevant here because we're checking the self-consistency of whole file, which additively will be consistent w/ GC results?
No description provided.