Skip to content

Commit

Permalink
objstorage: simplify AttachRemoteObjects
Browse files Browse the repository at this point in the history 
Simplify `AttachRemoteObjects` to not call `sharedSync`. We already
call `Sync` as part of ingestion.

Add a call `sharedSync` when closing the provider (this should be a
no-op in practice, other than tests).
  • Loading branch information
@RaduBerinde
RaduBerinde committed Jan 22, 2024
1 parent 8bdb7b1 commit 58981d1
Show file tree
Hide file tree
Showing 9 changed files with 13 additions and 31 deletions.
3 changes: 3 additions & 0 deletions objstorage/objstorage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,9 @@ type Provider interface {
CreateExternalObjectBacking(locator remote.Locator, objName string) (RemoteObjectBacking, error)

// AttachRemoteObjects registers existing remote objects with this provider.
//
// The objects are not guaranteed to be durable (accessible in case of
// crashes) until Sync is called.
AttachRemoteObjects(objs []RemoteObjectToAttach) ([]ObjectMetadata, error)

Close() error
Expand Down
8 changes: 6 additions & 2 deletions objstorage/objstorageprovider/provider.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -456,11 +456,15 @@ func (p *provider) Metrics() sharedcache.Metrics {
}

func (p *provider) addMetadata(meta objstorage.ObjectMetadata) {
p.mu.Lock()
defer p.mu.Unlock()
p.addMetadataLocked(meta)
}

func (p *provider) addMetadataLocked(meta objstorage.ObjectMetadata) {
if invariants.Enabled {
meta.AssertValid()
}
p.mu.Lock()
defer p.mu.Unlock()
p.mu.knownObjects[meta.DiskFileNum] = meta
if meta.IsRemote() {
p.mu.remote.catalogBatch.AddObject(remoteobjcat.RemoteObjectMetadata{
Expand Down
4 changes: 2 additions & 2 deletions objstorage/objstorageprovider/remote.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -126,9 +126,9 @@ func (p *provider) sharedClose() error {
if p.st.Remote.StorageFactory == nil {
return nil
}
var err error
err := p.sharedSync()
if p.remote.cache != nil {
err = p.remote.cache.Close()
err = firstError(err, p.remote.cache.Close())
p.remote.cache = nil
}
if p.remote.catalog != nil {
Expand Down
24 changes: 2 additions & 22 deletions objstorage/objstorageprovider/remote_backing.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/cockroachdb/errors"
"github.com/cockroachdb/pebble/internal/base"
"github.com/cockroachdb/pebble/objstorage"
"github.com/cockroachdb/pebble/objstorage/objstorageprovider/remoteobjcat"
"github.com/cockroachdb/pebble/objstorage/remote"
)

Expand Down Expand Up @@ -271,34 +270,15 @@ func (p *provider) AttachRemoteObjects(
}
}

func() {
p.mu.Lock()
defer p.mu.Unlock()
for _, d := range decoded {
p.mu.remote.catalogBatch.AddObject(remoteobjcat.RemoteObjectMetadata{
FileNum: d.meta.DiskFileNum,
FileType: d.meta.FileType,
CreatorID: d.meta.Remote.CreatorID,
CreatorFileNum: d.meta.Remote.CreatorFileNum,
CleanupMethod: d.meta.Remote.CleanupMethod,
Locator: d.meta.Remote.Locator,
CustomObjectName: d.meta.Remote.CustomObjectName,
})
}
}()
if err := p.sharedSync(); err != nil {
return nil, err
}

metas := make([]objstorage.ObjectMetadata, len(decoded))
for i, d := range decoded {
metas[i] = d.meta
}

p.mu.Lock()
defer p.mu.Unlock()
for _, meta := range metas {
p.mu.knownObjects[meta.DiskFileNum] = meta
for i := range metas {
p.addMetadataLocked(metas[i])
}
return metas, nil
}
1 change: 0 additions & 1 deletion objstorage/objstorageprovider/testdata/provider/shared_attach
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ b3 103
<remote> create object "eaac-1-000003.sst.ref.2.000103"
<remote> close writer for "eaac-1-000003.sst.ref.2.000103" after 0 bytes
<remote> size of object "eaac-1-000003.sst.ref.1.000003": 0
<local fs> sync: p2/REMOTE-OBJ-CATALOG-000001
000101 -> remote://61a6-1-000001.sst
000102 -> remote://a629-1-000002.sst
000103 -> remote://eaac-1-000003.sst
Expand Down
1 change: 0 additions & 1 deletion objstorage/objstorageprovider/testdata/provider/shared_attach_after_unref
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ p5b1 101
<remote> create object "d632-5-000001.sst.ref.6.000101"
<remote> close writer for "d632-5-000001.sst.ref.6.000101" after 0 bytes
<remote> size of object "d632-5-000001.sst.ref.5.000001": 0
<local fs> sync: p6/REMOTE-OBJ-CATALOG-000001
000101 -> remote://d632-5-000001.sst

switch p5
Expand Down
1 change: 0 additions & 1 deletion objstorage/objstorageprovider/testdata/provider/shared_attach_multi
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ b1 103
<remote> create object "61a6-1-000001.sst.ref.2.000103"
<remote> close writer for "61a6-1-000001.sst.ref.2.000103" after 0 bytes
<remote> size of object "61a6-1-000001.sst.ref.1.000001": 0
<local fs> sync: p2/REMOTE-OBJ-CATALOG-000001
000101 -> remote://61a6-1-000001.sst
000102 -> remote://61a6-1-000001.sst
000103 -> remote://61a6-1-000001.sst
Expand Down
1 change: 0 additions & 1 deletion objstorage/objstorageprovider/testdata/provider/shared_no_ref
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ attach
b1 101
b2 102
----
<local fs> sync: p2/REMOTE-OBJ-CATALOG-000001
000101 -> remote://61a6-1-000001.sst
000102 -> remote://61a6-1-000001.sst

Expand Down
1 change: 0 additions & 1 deletion objstorage/objstorageprovider/testdata/provider/shared_remove
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,6 @@ b2 102
<remote> create object "a629-1-000002.sst.ref.2.000102"
<remote> close writer for "a629-1-000002.sst.ref.2.000102" after 0 bytes
<remote> size of object "a629-1-000002.sst.ref.1.000002": 0
<local fs> sync: p2/REMOTE-OBJ-CATALOG-000001
000101 -> remote://61a6-1-000001.sst
000102 -> remote://a629-1-000002.sst

Expand Down

0 comments on commit 58981d1

Please sign in to comment.