Refactor block adapter walker #8575

N-o-Z · 2025-01-31T17:55:18Z

Change Description

Background

Remove usage of ingest store factory and catalog walker interface.
Catalog will now use underlying block adapter implementation of walker

github-actions · 2025-01-31T18:02:32Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2025-01-31T18:03:22Z

E2E Test Results - Quickstart

…-factory-walker-8570 # Conflicts: # pkg/catalog/catalog.go # pkg/ingest/store/factory.go

guy-har

Very nice clean up 🧹
I'm assuming our integration tests already test the import flows for all adapters, but I'm asking just to be on the safe side, how was this tested?

guy-har · 2025-02-05T17:28:24Z

pkg/block/local/adapter.go

 		return nil, err
 	}
-
-	err := VerifyAbsPath(uri.Path, l.path, l.allowedExternalPrefixes)
+	uriPath := strings.TrimSuffix(opts.StorageURI.Path, string(filepath.Separator))


I'm probably missing something
Why is this suddenly required?

This method was never called when creating a local walker so this validation never ran.
The Abs check is not really important, what is more important are the other checks for is in namespace and in allowed prefixes.

guy-har · 2025-02-05T17:30:00Z

pkg/block/azure/adapter.go

-	return NewAzureDataLakeWalker(client, false)
+	return NewAzureDataLakeWalker(client, opts.SkipOutOfOrder)


Why is this changed? bug you found along the way?

No - we just never used the GetWalker in that capacity. Now that we use it we need to propagate the value accordingly

guy-har · 2025-02-05T17:45:08Z

pkg/catalog/catalog.go

+			}
+			walker, err := block.NewWalkerWrapperFromAdapter(c.BlockAdapter, repository.StorageID.String(), source.Path, block.WalkerOptions{StorageURI: uri})
+			if err != nil {
+				return err


Suggested change

return err

return fmt.Errorf("creating object-store walker on path %s: %w", source.Path, err)

guy-har · 2025-02-05T17:46:26Z

pkg/catalog/catalog.go

+	}
+	walker, err := block.NewWalkerWrapperFromAdapter(c.BlockAdapter, repository.StorageID.String(), params.SourceURI, block.WalkerOptions{StorageURI: uri, SkipOutOfOrder: true})
+	if err != nil {
+		return nil, nil, err


Suggested change

return nil, nil, err

return nil, nil, fmt.Errorf("creating object-store walker: %w", err)

guy-har · 2025-02-05T17:46:54Z

pkg/catalog/catalog.go

@@ -2504,10 +2499,13 @@ func (c *Catalog) WriteRange(ctx context.Context, repositoryID string, params Wr
 		return nil, nil, err
 	}

-	// TODO (niro): Need to handle this at some point (use adapter GetWalker)


guy-har · 2025-02-05T18:51:19Z

pkg/catalog/catalog.go

@@ -412,13 +405,13 @@ func New(ctx context.Context, cfg Config) (*Catalog, error) {
 	workPool := pond.New(sharedWorkers, sharedWorkers*pendingTasksPerWorker, pond.Context(ctx))

 	return &Catalog{
+		Config:                cfg.Config,


Why is this is this needed? it's not used anywhere

Another PR - removed

pkg/block/adapter.go

guy-har · 2025-02-05T19:06:29Z

pkg/block/adapter.go

+func NewWalkerWrapper(walker Walker, uri *url.URL) *WalkerWrapper {
+	return &WalkerWrapper{
+		walker: walker,
+		uri:    uri,
+	}
+}


Why is this needed? I only see calls from tests

Only for tests

guy-har · 2025-02-05T19:13:43Z

pkg/block/adapter.go

+func NewWalkerWrapperFromAdapter(adapter Adapter, storageID, path string, opts WalkerOptions) (*WalkerWrapper, error) {
+	walker, err := adapter.GetWalker(storageID, opts)
+	if err != nil {
+		return nil, fmt.Errorf("creating object-store walker on path %s: %w", path, err)
+	}
+	return &WalkerWrapper{
+		walker: walker,
+		uri:    opts.StorageURI,
+	}, nil
+}


It feels like to many ways to initiate the WalkerWrapper, IMO leaving only NewWalkerWrapper is nicer, NewWalkerWrapperFromAdapter not worth the 1 line of code it reduces. It's a matter of taste though

guy-har · 2025-02-05T19:22:06Z

esti/lakectl_test.go

@@ -867,6 +867,7 @@ func TestLakectlFsStat(t *testing.T) {

 func TestLakectlImport(t *testing.T) {
 	// TODO(barak): generalize test to work all supported object stores
+	const IngestTestBucketPath = "s3://esti-system-testing-data/ingest-test-data/"


nit: move up or use inline

N-o-Z · 2025-02-05T21:43:42Z

Very nice clean up 🧹 I'm assuming our integration tests already test the import flows for all adapters, but I'm asking just to be on the safe side, how was this tested?

The existing esti tests cover the changes for all blockstore types

guy-har

Looks good!
Approving, but please remove unused config

pkg/block/adapter.go

guy-har · 2025-02-06T08:10:21Z

pkg/block/adapter.go

+func NewWalkerWrapperFromAdapter(adapter Adapter, storageID, path string, opts WalkerOptions) (*WalkerWrapper, error) {
+	walker, err := adapter.GetWalker(storageID, opts)
+	if err != nil {
+		return nil, fmt.Errorf("creating object-store walker on path %s: %w", path, err)
+	}
+	return &WalkerWrapper{
+		walker: walker,
+		uri:    opts.StorageURI,
+	}, nil
+}


This can be removed

guy-har · 2025-02-06T08:13:25Z

pkg/catalog/catalog.go

 	SettingsManagerOption settings.ManagerOption
 	PathProvider          *upload.PathPartitionProvider
 }

 type Catalog struct {
+	Config                config.Config


It's still here

Co-authored-by: guy-har <[email protected]>

Refactor block adapter walker

ad851e2

N-o-Z added exclude-changelog PR description should not be included in next release changelog msb labels Jan 31, 2025

N-o-Z requested review from guy-har and a team January 31, 2025 17:55

N-o-Z self-assigned this Jan 31, 2025

Merge remote-tracking branch 'origin/master' into task/catalog-remove…

9f37ebd

…-factory-walker-8570 # Conflicts: # pkg/catalog/catalog.go # pkg/ingest/store/factory.go

guy-har requested changes Feb 5, 2025

View reviewed changes

CR Fixes

012f1c7

N-o-Z requested a review from guy-har February 5, 2025 21:43

guy-har approved these changes Feb 6, 2025

View reviewed changes

N-o-Z and others added 2 commits February 6, 2025 11:24

CR Fixes

7c4a6ea

Update pkg/block/adapter.go

1514b67

Co-authored-by: guy-har <[email protected]>

N-o-Z enabled auto-merge (squash) February 6, 2025 16:25

N-o-Z merged commit 7017f27 into master Feb 6, 2025
38 checks passed

N-o-Z deleted the task/catalog-remove-factory-walker-8570 branch February 6, 2025 16:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor block adapter walker #8575

Refactor block adapter walker #8575

N-o-Z commented Jan 31, 2025

github-actions bot commented Jan 31, 2025

github-actions bot commented Jan 31, 2025 •

edited

Loading

guy-har left a comment

guy-har Feb 5, 2025

N-o-Z Feb 5, 2025

guy-har Feb 5, 2025

N-o-Z Feb 5, 2025

guy-har Feb 5, 2025

guy-har Feb 5, 2025

guy-har Feb 5, 2025

guy-har Feb 5, 2025

N-o-Z Feb 5, 2025

guy-har Feb 5, 2025

N-o-Z Feb 5, 2025

guy-har Feb 5, 2025

N-o-Z Feb 5, 2025

guy-har Feb 5, 2025

N-o-Z commented Feb 5, 2025

guy-har left a comment

guy-har Feb 6, 2025

guy-har Feb 6, 2025

		return NewAzureDataLakeWalker(client, false)
		return NewAzureDataLakeWalker(client, opts.SkipOutOfOrder)

	return err
	return fmt.Errorf("creating object-store walker on path %s: %w", source.Path, err)

	return nil, nil, err
	return nil, nil, fmt.Errorf("creating object-store walker: %w", err)

Refactor block adapter walker #8575

Refactor block adapter walker #8575

Conversation

N-o-Z commented Jan 31, 2025

Change Description

Background

github-actions bot commented Jan 31, 2025

E2E Test Results - DynamoDB Local - Local Block Adapter

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

E2E Test Results - Quickstart

guy-har 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

Choose a reason for hiding this comment

N-o-Z commented Feb 5, 2025

guy-har left a 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 31, 2025 •

edited

Loading