Add StorageID to ObjectPointer #8522

itaigilo · 2025-01-20T21:45:31Z

Closes #8496.

Change Description

Adding StorageID to ObjectPointer.
Adding StorageID to the init of ObjectPointer, that cover the basic flows of the server.

My approach in this PR

Search for all initializations of ObjectPointer, and:

Used repo.StorageID where possible.
Added a TODO if repo isn't present.
Added "" if it's a test.

What's Next

I'll open new Issues for the following, to be handled later on:

Add StorageID to objPointer of TierFS.
Add StorageID to GarbageCollectionManager.
Add StorageID to ResolveNamespace:
- This is used by GC, so first we need to understand how to handle the GC.
- Is DefaultResolveNamespace relevant to StorageID?
Re-search for value-reads of ObjectPointer.StorageNamespace, and see if they need ObjectID references.
Handle ensureStorageNamespace (it probably need to be moved to the Block interface, and add storageID to its signature).
Handle actions: OutputWrite should have StorageID, and so does HookOutputWriter.

…e-id-to-repo-endpoints

This reverts commit 298bc0d.

This reverts commit 299c292.

…e-id-to-repo-endpoints

github-actions · 2025-01-20T21:46:57Z

🎊 PR Preview b361b34 has been successfully built and deployed to https://treeverse-lakeFS-preview-pr-8522.surge.sh

🕐 Build time: 0.01s

_{🤖 By surge-preview}

github-actions · 2025-01-20T21:53:25Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-01-20T21:53:48Z

E2E Test Results - Quickstart

itaigilo · 2025-01-20T22:05:12Z

modules/block/factory/go.work.sum

No clue why it wasn't added before, but I think it should be in master.

Why not in it's own PR then?

Following a discussion with @guy-har -
I'll remove from here it once #8527 will be merged to master (to avoid annoying conflicts).

nadavsteindler · 2025-01-21T09:45:25Z

pkg/api/controller.go

@@ -2091,6 +2096,7 @@ func (c *Controller) ensureStorageNamespace(ctx context.Context, storageNamespac
 	// this serves two purposes, first, we maintain safety check for older lakeFS version.
 	// second, in scenarios where lakeFS shouldn't have access to the root namespace (i.e pre-sign URL only).
 	if c.Config.Graveler.EnsureReadableRootNamespace {
+		// TODO (gilo): ObjectPointer init - add StorageID here


Why was this change delayed? It's going in a different PR?

To reduce the scope of this one, and making it more reviewable (yet complete enough to be able to merge it in the end).

nadavsteindler · 2025-01-21T09:46:47Z

pkg/api/controller.go

 	var blob *upload.Blob
 	if mediaType != "multipart/form-data" {
 		// handle non-multipart, direct content upload
 		address := c.PathProvider.NewPath()
-		blob, err = upload.WriteBlob(ctx, c.BlockAdapter, repo.StorageNamespace, address, r.Body, r.ContentLength,


Does this code style change indicate a change in our best practices?

That's a good question. I don't think we have a code style guide for go,
And the formatter didn't seem to care.
These lines became too long.

nadavsteindler · 2025-01-21T09:47:57Z

pkg/api/controller_test.go

@@ -2550,7 +2550,7 @@ func TestController_ObjectsHeadObjectHandler(t *testing.T) {
 	buf := new(bytes.Buffer)
 	buf.WriteString("this is file content made up of bytes")
 	address := upload.DefaultPathProvider.NewPath()
-	blob, err := upload.WriteBlob(context.Background(), deps.blocks, onBlock(deps, "ns1"), address, buf, 37, block.PutOpts{StorageClass: &expensiveString})
+	blob, err := upload.WriteBlob(context.Background(), deps.blocks, "", onBlock(deps, "ns1"), address, buf, 37, block.PutOpts{StorageClass: &expensiveString})


Is it valid to provide an empty string for the storageid? Shouldn't this trigger some sort of "invalid parameter" exception?

Currently, empty StorageID is the only valid option.

nadavsteindler · 2025-01-21T09:52:20Z

pkg/block/adapter.go

@@ -63,6 +63,7 @@ const DefaultPreSignExpiryDuration = 15 * time.Minute
 // ObjectPointer is a unique identifier of an object in the object
 // store: the store is a 1:1 mapping between pointers and objects.
 type ObjectPointer struct {
+	StorageID        string


Documentation: This is such an important core class class, but there is no explanation what sort of values these fields are ment to take. Is "S3" a valid storage ID? Or maybe a URL is expected like "https://bucketname.s3.amazonaws.com"? I have no idea when reading this code?

Same goes for the other two fields

This is out of scope for this PR - we should however do this for the config part and document that well.
@itaigilo This is another thing we need to bring up with product

nadavsteindler · 2025-01-21T09:56:21Z

pkg/gateway/operations/putobject_test.go

-	bucketName      = "test"
-	ObjectBlockSize = 1024 * 3
+	storageNamespace = "test"
+	ObjectBlockSize  = 1024 * 3


Is this size measured in bits? bytes? gigabytes? Would be nice if I didn't have to guess(const name or docs)

No clue -
only changed whitespaces here...

nadavsteindler · 2025-01-21T09:57:49Z

pkg/gateway/operations/getobject.go

@@ -110,6 +110,7 @@ func (controller *GetObject) Handle(w http.ResponseWriter, req *http.Request, o
 	contentLength := entry.Size
 	contentRange := ""
 	objectPointer := block.ObjectPointer{
+		StorageID:        o.Repository.StorageID,


I assume these will be "" fore existing repos? Is that valid? Don't we need a migrator to initialize the values for existing repos? Is that in this PR? Another issue?

Yes, "" is valid, for existing repos.

N-o-Z

Thanks - most of it is pretty straightforward.
I am however asking for changes on some of the open issues + some cleanups.

N-o-Z · 2025-01-21T17:49:40Z

pkg/upload/write_blob.go

@@ -17,11 +17,12 @@ type Blob struct {
 	CreationDate    time.Time
 }

-func WriteBlob(ctx context.Context, adapter block.Adapter, bucketName, address string, body io.Reader, contentLength int64, opts block.PutOpts) (*Blob, error) {
+func WriteBlob(ctx context.Context, adapter block.Adapter, storageID, storageNamespace, address string, body io.Reader, contentLength int64, opts block.PutOpts) (*Blob, error) {


At this point I think we should just consider passing the ObjectPointer instead of all these params

N-o-Z · 2025-01-21T17:51:19Z

pkg/pyramid/tier_fs.go

@@ -419,6 +419,7 @@ func (tfs *TierFS) newLocalFileRef(namespace, nsPath, filename string) localFile
 }

 func (tfs *TierFS) objPointer(namespace, filename string) block.ObjectPointer {
+	// TODO (gilo): ObjectPointer init - add StorageID here


How are we planning on doing that?

Basically we need to propagate this in the hierarchy -
I'll soon do this in a separate PR, to not overload this PR.

N-o-Z · 2025-01-21T17:58:10Z

pkg/catalog/catalog_test.go

 	objReader, err := c.BlockAdapter.Get(ctx, block.ObjectPointer{
 		Identifier:       obj,
 		IdentifierType:   block.IdentifierTypeFull,
+		StorageID:        "",


Not necessary - can be removed

N-o-Z · 2025-01-21T18:00:57Z

pkg/api/controller.go

+	storageNamespace := repo.StorageNamespace
+	storageID := repo.StorageID
+


Redundant - please remove

N-o-Z · 2025-01-21T18:02:40Z

pkg/block/adapter.go

@@ -63,6 +63,7 @@ const DefaultPreSignExpiryDuration = 15 * time.Minute
 // ObjectPointer is a unique identifier of an object in the object
 // store: the store is a 1:1 mapping between pointers and objects.
 type ObjectPointer struct {
+	StorageID        string


This is out of scope for this PR - we should however do this for the config part and document that well.
@itaigilo This is another thing we need to bring up with product

N-o-Z · 2025-01-21T18:04:53Z

pkg/block/azure/adapter.go

@@ -139,6 +139,7 @@ func ResolveBlobURLInfoFromURL(pathURL *url.URL) (BlobURLInfo, error) {

 func resolveBlobURLInfo(obj block.ObjectPointer) (BlobURLInfo, error) {
 	key := obj.Identifier
+	// TODO (gilo): verify there's no need for StorageID here


At this point you are already in the context of a specific adapter instance so storage ID is not needed here

N-o-Z · 2025-01-21T18:06:12Z

pkg/block/s3/adapter.go

@@ -411,6 +411,7 @@ func (a *Adapter) GetPreSignedURL(ctx context.Context, obj block.ObjectPointer,

 	log := a.log(ctx).WithFields(logging.Fields{
 		"operation":  "GetPreSignedURL",
+		"sid":        obj.StorageID, // TODO (gilo): is "sid" a good enough name?


Definitely not good enough :)
Let's be explicit (we don't have to be stingy in our logs)

The main reason I haven't done this is that StorageNamespace is logged as namespace and not as storage_namespace 😖

N-o-Z · 2025-01-21T18:06:51Z

pkg/block/s3/adapter.go

@@ -466,6 +467,7 @@ func (a *Adapter) GetPresignUploadPartURL(ctx context.Context, obj block.ObjectP

 	log := a.log(ctx).WithFields(logging.Fields{
 		"operation":  "GetPresignUploadPartURL",
+		"sid":        obj.StorageID, // TODO (gilo): is "sid" a good enough name?


Same as above

N-o-Z · 2025-01-21T18:07:50Z

pkg/catalog/catalog.go

@@ -2630,7 +2630,7 @@ type UncommittedParquetObject struct {
 	CreationDate    int64  `parquet:"name=creation_date, type=INT64, convertedtype=INT_64"`
 }

-func (c *Catalog) uploadFile(ctx context.Context, ns graveler.StorageNamespace, location string, fd *os.File, size int64) (string, error) {
+func (c *Catalog) uploadFile(ctx context.Context, sid graveler.StorageID, ns graveler.StorageNamespace, location string, fd *os.File, size int64) (string, error) {


Please don't use sid, it's overloaded and confusing

Following the ns "convention".

N-o-Z · 2025-01-21T18:09:21Z

pkg/catalog/catalog.go

@@ -2630,7 +2630,7 @@ type UncommittedParquetObject struct {
 	CreationDate    int64  `parquet:"name=creation_date, type=INT64, convertedtype=INT_64"`
 }

-func (c *Catalog) uploadFile(ctx context.Context, ns graveler.StorageNamespace, location string, fd *os.File, size int64) (string, error) {
+func (c *Catalog) uploadFile(ctx context.Context, sid graveler.StorageID, ns graveler.StorageNamespace, location string, fd *os.File, size int64) (string, error) {


At this point consider just passing the repository object, will be cleaner than just adding more params

itaigilo

@N-o-Z fixed, PTAL again.

itaigilo · 2025-01-22T09:18:02Z

pkg/block/s3/adapter.go

@@ -411,6 +411,7 @@ func (a *Adapter) GetPreSignedURL(ctx context.Context, obj block.ObjectPointer,

 	log := a.log(ctx).WithFields(logging.Fields{
 		"operation":  "GetPreSignedURL",
+		"sid":        obj.StorageID, // TODO (gilo): is "sid" a good enough name?


The main reason I haven't done this is that StorageNamespace is logged as namespace and not as storage_namespace 😖

itaigilo · 2025-01-22T09:22:00Z

pkg/catalog/catalog.go

@@ -2630,7 +2630,7 @@ type UncommittedParquetObject struct {
 	CreationDate    int64  `parquet:"name=creation_date, type=INT64, convertedtype=INT_64"`
 }

-func (c *Catalog) uploadFile(ctx context.Context, ns graveler.StorageNamespace, location string, fd *os.File, size int64) (string, error) {
+func (c *Catalog) uploadFile(ctx context.Context, sid graveler.StorageID, ns graveler.StorageNamespace, location string, fd *os.File, size int64) (string, error) {


Following the ns "convention".

itaigilo · 2025-01-22T09:24:59Z

pkg/pyramid/tier_fs.go

@@ -419,6 +419,7 @@ func (tfs *TierFS) newLocalFileRef(namespace, nsPath, filename string) localFile
 }

 func (tfs *TierFS) objPointer(namespace, filename string) block.ObjectPointer {
+	// TODO (gilo): ObjectPointer init - add StorageID here


Basically we need to propagate this in the hierarchy -
I'll soon do this in a separate PR, to not overload this PR.

N-o-Z

Beautiful, thanks!!

N-o-Z · 2025-01-22T16:39:52Z

pkg/api/controller.go

-					-1,
-					block.PutOpts{StorageClass: params.StorageClass},
-				)
+				objectPointer := block.ObjectPointer{


N-o-Z · 2025-01-22T16:42:03Z

pkg/catalog/catalog.go

@@ -2630,7 +2630,7 @@ type UncommittedParquetObject struct {
 	CreationDate    int64  `parquet:"name=creation_date, type=INT64, convertedtype=INT_64"`
 }

-func (c *Catalog) uploadFile(ctx context.Context, sid graveler.StorageID, ns graveler.StorageNamespace, location string, fd *os.File, size int64) (string, error) {
+func (c *Catalog) uploadFile(ctx context.Context, repo *graveler.RepositoryRecord, location string, fd *os.File, size int64) (string, error) {


N-o-Z · 2025-01-22T16:43:18Z

pkg/block/s3/adapter.go

@@ -411,7 +411,7 @@ func (a *Adapter) GetPreSignedURL(ctx context.Context, obj block.ObjectPointer,

 	log := a.log(ctx).WithFields(logging.Fields{
 		"operation":  "GetPreSignedURL",
-		"sid":        obj.StorageID, // TODO (gilo): is "sid" a good enough name?
+		"storage_id": obj.StorageID,


# Conflicts: # pkg/api/controller.go

itaigilo added 29 commits January 15, 2025 13:41

Add StorageID to Repository entity

c1d3587

Bug fixes

4e51981

Remove API changes

30b3849

Add StorageID to Repo endpoints

b45d428

Almost empty commit

02e4404

Revert almso empty commit

74e2202

Remove from API

222b126

Merge branch 'feature/add-storage-id-to-repo' into feature/add-storag…

32b273f

…e-id-to-repo-endpoints

Add param to Creation

65974c0

Fix PR comments

af9476e

Fix tests

4e8e423

Merge branch 'feature/add-storage-id-to-repo' into feature/add-storag…

5eebff2

…e-id-to-repo-endpoints

Add basic unit-tests

478d498

Fix param

13a771e

Update tests

989a388

Allow only empty StorageID

298bc0d

Revert "Allow only empty StorageID"

299c292

This reverts commit 298bc0d.

Revert "Revert "Allow only empty StorageID""

b81eff3

This reverts commit 299c292.

Change validation

acb70e8

Remove StorageID in a test

3dbd9d1

Merge branch 'feature/add-storage-id-to-repo' into feature/add-storag…

aa8180a

…e-id-to-repo-endpoints

Update tests

cecce8d

Merge branch 'master' into feature/add-storage-id-to-repo

1ced59b

Merge branch 'feature/add-storage-id-to-repo' into feature/add-storag…

858812c

…e-id-to-repo-endpoints

Add StorageID to ObjectPointer inits

df0b2af

Add StorageID to WriteBlob

920cd0f

Add StorageID to uploadFile

7386b7e

Add log fields

6f8d83a

Add comments

b361b34

itaigilo added the exclude-changelog PR description should not be included in next release changelog label Jan 20, 2025

itaigilo added the msb label Jan 20, 2025

itaigilo requested a review from N-o-Z January 20, 2025 21:45

itaigilo requested review from arielshaqed, nadavsteindler and guy-har January 20, 2025 21:53

Merge branch 'master' into feature/add-storage-id-to-obj-ptr-part-1

428f57a

itaigilo commented Jan 20, 2025

View reviewed changes

nadavsteindler reviewed Jan 21, 2025

View reviewed changes

nadavsteindler requested changes Jan 21, 2025

View reviewed changes

itaigilo added 2 commits January 21, 2025 18:08

Merge branch 'master' into feature/add-storage-id-to-obj-ptr-part-1

3221718

Remove go.work.sum

a66dba0

nadavsteindler approved these changes Jan 21, 2025

View reviewed changes

N-o-Z requested changes Jan 21, 2025

View reviewed changes

Fix PR comments

789033d

itaigilo commented Jan 22, 2025

View reviewed changes

itaigilo requested a review from N-o-Z January 22, 2025 09:37

N-o-Z approved these changes Jan 22, 2025

View reviewed changes

Merge branch 'master' into feature/add-storage-id-to-obj-ptr-part-1

b0a76ba

# Conflicts: # pkg/api/controller.go

itaigilo enabled auto-merge (squash) January 22, 2025 21:06

itaigilo merged commit 49becea into master Jan 22, 2025
38 checks passed

itaigilo deleted the feature/add-storage-id-to-obj-ptr-part-1 branch January 22, 2025 21:09

		storageNamespace := repo.StorageNamespace
		storageID := repo.StorageID

Add StorageID to ObjectPointer #8522

Add StorageID to ObjectPointer #8522

Conversation

itaigilo commented Jan 20, 2025

Change Description

My approach in this PR

What's Next

github-actions bot commented Jan 20, 2025 • edited Loading

github-actions bot commented Jan 20, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Jan 20, 2025

E2E Test Results - Quickstart

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 20, 2025 •

edited

Loading