From 5b280af78f31791bf0e5701c9e649e73fe6f5c82 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Mon, 22 Jan 2024 13:49:51 -0800 Subject: [PATCH] objstorage: simplify AttachRemoteObjects 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). --- objstorage/objstorage.go | 3 +++ objstorage/objstorageprovider/provider.go | 8 +++++-- objstorage/objstorageprovider/remote.go | 4 ++-- .../objstorageprovider/remote_backing.go | 24 ++----------------- .../testdata/provider/shared_attach | 1 - .../provider/shared_attach_after_unref | 1 - .../testdata/provider/shared_attach_multi | 1 - .../testdata/provider/shared_no_ref | 1 - .../testdata/provider/shared_remove | 1 - 9 files changed, 13 insertions(+), 31 deletions(-) diff --git a/objstorage/objstorage.go b/objstorage/objstorage.go index 9805fb144e..6728b1d46b 100644 --- a/objstorage/objstorage.go +++ b/objstorage/objstorage.go @@ -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 diff --git a/objstorage/objstorageprovider/provider.go b/objstorage/objstorageprovider/provider.go index bd6f2038b7..bc42d85351 100644 --- a/objstorage/objstorageprovider/provider.go +++ b/objstorage/objstorageprovider/provider.go @@ -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{ diff --git a/objstorage/objstorageprovider/remote.go b/objstorage/objstorageprovider/remote.go index 70fdfc78ec..dd1c99c675 100644 --- a/objstorage/objstorageprovider/remote.go +++ b/objstorage/objstorageprovider/remote.go @@ -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 { diff --git a/objstorage/objstorageprovider/remote_backing.go b/objstorage/objstorageprovider/remote_backing.go index 8e184acc00..964104784f 100644 --- a/objstorage/objstorageprovider/remote_backing.go +++ b/objstorage/objstorageprovider/remote_backing.go @@ -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" ) @@ -271,25 +270,6 @@ 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 @@ -297,8 +277,8 @@ func (p *provider) AttachRemoteObjects( 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 } diff --git a/objstorage/objstorageprovider/testdata/provider/shared_attach b/objstorage/objstorageprovider/testdata/provider/shared_attach index fac25a8a0b..799e8ccf0e 100644 --- a/objstorage/objstorageprovider/testdata/provider/shared_attach +++ b/objstorage/objstorageprovider/testdata/provider/shared_attach @@ -101,7 +101,6 @@ b3 103 create object "eaac-1-000003.sst.ref.2.000103" close writer for "eaac-1-000003.sst.ref.2.000103" after 0 bytes size of object "eaac-1-000003.sst.ref.1.000003": 0 - sync: p2/REMOTE-OBJ-CATALOG-000001 000101 -> remote://61a6-1-000001.sst 000102 -> remote://a629-1-000002.sst 000103 -> remote://eaac-1-000003.sst diff --git a/objstorage/objstorageprovider/testdata/provider/shared_attach_after_unref b/objstorage/objstorageprovider/testdata/provider/shared_attach_after_unref index 719737cbcf..234773f7fd 100644 --- a/objstorage/objstorageprovider/testdata/provider/shared_attach_after_unref +++ b/objstorage/objstorageprovider/testdata/provider/shared_attach_after_unref @@ -45,7 +45,6 @@ p5b1 101 create object "d632-5-000001.sst.ref.6.000101" close writer for "d632-5-000001.sst.ref.6.000101" after 0 bytes size of object "d632-5-000001.sst.ref.5.000001": 0 - sync: p6/REMOTE-OBJ-CATALOG-000001 000101 -> remote://d632-5-000001.sst switch p5 diff --git a/objstorage/objstorageprovider/testdata/provider/shared_attach_multi b/objstorage/objstorageprovider/testdata/provider/shared_attach_multi index 93baa318ac..a9e4aa72c6 100644 --- a/objstorage/objstorageprovider/testdata/provider/shared_attach_multi +++ b/objstorage/objstorageprovider/testdata/provider/shared_attach_multi @@ -49,7 +49,6 @@ b1 103 create object "61a6-1-000001.sst.ref.2.000103" close writer for "61a6-1-000001.sst.ref.2.000103" after 0 bytes size of object "61a6-1-000001.sst.ref.1.000001": 0 - sync: p2/REMOTE-OBJ-CATALOG-000001 000101 -> remote://61a6-1-000001.sst 000102 -> remote://61a6-1-000001.sst 000103 -> remote://61a6-1-000001.sst diff --git a/objstorage/objstorageprovider/testdata/provider/shared_no_ref b/objstorage/objstorageprovider/testdata/provider/shared_no_ref index 6c16f9af2c..c716694eae 100644 --- a/objstorage/objstorageprovider/testdata/provider/shared_no_ref +++ b/objstorage/objstorageprovider/testdata/provider/shared_no_ref @@ -134,7 +134,6 @@ attach b1 101 b2 102 ---- - sync: p2/REMOTE-OBJ-CATALOG-000001 000101 -> remote://61a6-1-000001.sst 000102 -> remote://61a6-1-000001.sst diff --git a/objstorage/objstorageprovider/testdata/provider/shared_remove b/objstorage/objstorageprovider/testdata/provider/shared_remove index f9c52a1497..f7a4d63a2d 100644 --- a/objstorage/objstorageprovider/testdata/provider/shared_remove +++ b/objstorage/objstorageprovider/testdata/provider/shared_remove @@ -66,7 +66,6 @@ b2 102 create object "a629-1-000002.sst.ref.2.000102" close writer for "a629-1-000002.sst.ref.2.000102" after 0 bytes size of object "a629-1-000002.sst.ref.1.000002": 0 - sync: p2/REMOTE-OBJ-CATALOG-000001 000101 -> remote://61a6-1-000001.sst 000102 -> remote://a629-1-000002.sst