From 14e2c0e7eab9349a4a0e9d79884866147b899d79 Mon Sep 17 00:00:00 2001 From: Aaron Son Date: Mon, 10 Feb 2025 17:13:49 -0800 Subject: [PATCH 1/2] [no-release-notes] go: nbs,remotesrv: Cleanup and comments for AddTableFilesToManifest ref checks PR. --- go/libraries/doltcore/remotesrv/grpc.go | 5 +++++ go/store/nbs/journal.go | 8 ++++++++ go/store/nbs/table_set.go | 13 ++++++++++--- 3 files changed, 23 insertions(+), 3 deletions(-) diff --git a/go/libraries/doltcore/remotesrv/grpc.go b/go/libraries/doltcore/remotesrv/grpc.go index 4f29d2041c..c675fa099b 100644 --- a/go/libraries/doltcore/remotesrv/grpc.go +++ b/go/libraries/doltcore/remotesrv/grpc.go @@ -654,6 +654,11 @@ func (rs *RemoteChunkStore) AddTableFiles(ctx context.Context, req *remotesapi.A return &remotesapi.AddTableFilesResponse{Success: true}, nil } +// Returns a |chunks.GetAddrsCurry| for the nbf (NomsBinFormat) +// corresponding to |version|. +// +// Used to implement chunk reference sanity checks when adding table files that have +// been uploaded by clients to the stores managed by the gRPC server. func (rs *RemoteChunkStore) getAddrs(version string) chunks.GetAddrsCurry { fmt, err := types.GetFormatForVersionString(version) if err != nil { diff --git a/go/store/nbs/journal.go b/go/store/nbs/journal.go index e33c8614ae..7bbb4fd6d3 100644 --- a/go/store/nbs/journal.go +++ b/go/store/nbs/journal.go @@ -195,6 +195,14 @@ func trueUpBackingManifest(ctx context.Context, root hash.Hash, backing *journal if err != nil { return manifestContents{}, err } else if !ok { + // If there is no backing manifest yet, we simply + // return without any manifest contents. We can open a + // newly created (cloned) journal file before the + // manifest corresponding to its existence has been + // created. |*ChunkJournal.ParseIfExists| forwards + // to the backing store in the case that the loaded + // manifest is currently empty, so eventually the + // manifest will be created. return manifestContents{}, nil } diff --git a/go/store/nbs/table_set.go b/go/store/nbs/table_set.go index fb495733c4..63ada060e9 100644 --- a/go/store/nbs/table_set.go +++ b/go/store/nbs/table_set.go @@ -440,6 +440,8 @@ func (ts tableSet) openForAdd(ctx context.Context, files map[hash.Hash]uint32, s source.close() } } + // First add clones of all sources that are already present in + // ts.novel or ts.upstream. for h := range files { if s, ok := ts.novel[h]; ok { cloned, err := s.clone() @@ -457,16 +459,21 @@ func (ts tableSet) openForAdd(ctx context.Context, files map[hash.Hash]uint32, s ret[h] = cloned } } + // Concurrently open all files that are not already + // in |ret|. eg, ctx := errgroup.WithContext(ctx) var mu sync.Mutex - for h, c := range files { + for fileId, chunkCount := range files { + if _, ok := ret[fileId]; ok { + continue + } eg.Go(func() error { - cs, err := ts.p.Open(ctx, h, c, stats) + cs, err := ts.p.Open(ctx, fileId, chunkCount, stats) if err != nil { return err } mu.Lock() - ret[h] = cs + ret[fileId] = cs mu.Unlock() return nil }) From 17d5e2400c1218596d87c0860dfea328703f5a38 Mon Sep 17 00:00:00 2001 From: Aaron Son Date: Mon, 10 Feb 2025 18:23:47 -0800 Subject: [PATCH 2/2] go/store/nbs: table_set.go: Fix race on ret map in openForAdd. --- go/store/nbs/table_set.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/go/store/nbs/table_set.go b/go/store/nbs/table_set.go index 63ada060e9..b897b3068d 100644 --- a/go/store/nbs/table_set.go +++ b/go/store/nbs/table_set.go @@ -464,7 +464,10 @@ func (ts tableSet) openForAdd(ctx context.Context, files map[hash.Hash]uint32, s eg, ctx := errgroup.WithContext(ctx) var mu sync.Mutex for fileId, chunkCount := range files { - if _, ok := ret[fileId]; ok { + mu.Lock() + _, ok := ret[fileId] + mu.Unlock() + if ok { continue } eg.Go(func() error {