Delta Exporter: Azure Support #7444

N-o-Z · 2024-02-06T20:55:36Z

Closes #7296

Change Description

Background

Add support for delta catalog export with Azure

Testing Details

Added new esti test for delta catalog export with AWS and Azure falvors

Breaking Change?

No

github-actions · 2024-02-06T20:56:45Z

♻️ PR Preview 1032728 has been successfully destroyed since this PR has been closed.

_{🤖 By surge-preview}

github-actions · 2024-02-06T21:02:36Z

E2E Test Results - DynamoDB Local - Local Block Adapter

itaiad200

Always great to see test coverage added, thank you! I enjoyed reading this, few questions and requests for clarifications as I'm not very familiar with our exports.

itaiad200 · 2024-02-08T13:54:53Z

pkg/actions/lua/storage/azure/client.go

+// ListObjects Should be implemented when needed. There are nuances between HNS and BlobStorage which requires understanding the
+// Actual use case before implementing the solution
+func (c *Client) ListObjects(l *lua.State) int {
+	lua.Errorf(l, "Not implemented")
+	panic("unreachable")
+}
+
+// DeleteRecursive Should be implemented when needed. There are nuances between HNS and BlobStorage which requires understanding the
+// Actual use case before implementing the solution
+func (c *Client) DeleteRecursive(l *lua.State) int {
+	lua.Errorf(l, "Not implemented")
+	panic("unreachable")
+}


Not sure how it works, but if this client implements an interface, wouldn't it be failing without the user understand why it was not implemented?
The user wants to export tables, can it work without these 2? Don't we delete objects once we delete a branch for example?

itaiad200 · 2024-02-08T14:01:09Z

pkg/actions/lua.go

+			Script:        script,
+			Args:          args,
+			collector:     collector,
+			serverAddress: serverAddress,


What's this?

lakeFS server address - used to create the lakeFS client in the lua script

itaiad200 · 2024-02-08T14:05:45Z

docs/howto/hooks/lua.md

@@ -412,18 +412,18 @@ Parameters:

 A package used to export Delta Lake tables from lakeFS to an external cloud storage.

-### `lakefs/catalogexport/delta_exporter.export_delta_log(action, table_names, writer, delta_client, table_descriptors_path)`
+### `lakefs/catalogexport/delta_exporter.export_delta_log(action, table_def_names, write_object, delta_client, table_descriptors_path)`


Is this a breaking change? I'm not very familiar with named params in lua

This was a bug in the documentation - the code did not change

itaiad200 · 2024-02-08T14:19:22Z

esti/export_hooks_files/delta/s3/_lakefs_actions/delta_export.yaml

+        local aws = require("aws")
+        local formats = require("formats")
+        local delta_exporter = require("lakefs/catalogexport/delta_exporter")
+
+        local table_descriptors_path = "_lakefs_tables"
+        local sc = aws.s3_client(args.aws.access_key_id, args.aws.secret_access_key, args.aws.region)
+        local delta_client = formats.delta_client(args.lakefs.access_key_id, args.lakefs.secret_access_key, args.aws.region)
+        local delta_table_locations = delta_exporter.export_delta_log(action, args.table_names, sc.put_object, delta_client, table_descriptors_path)
+
+        for t, loc in pairs(delta_table_locations) do
+          print("Delta Lake exported table \"" .. t .. "\"'s location: " .. loc .. "\n")
+        end


Why are we copying the script here and not utilizing the higher level lua func?

Not sure I understand the comment - can you explain?

itaiad200 · 2024-02-08T16:30:30Z

esti/export_hooks_files/delta/azure/_lakefs_actions/delta_export.yaml

+        local table_descriptors_path = "_lakefs_tables"
+        local sc = azure.client(args.azure.storage_account, args.azure.access_key)
+        local delta_client = formats.delta_client(args.lakefs.access_key_id, args.lakefs.secret_access_key)
+        local delta_table_locations = delta_exporter.export_delta_log(action, args.table_names, sc.put_object, delta_client, table_descriptors_path)
+
+        for t, loc in pairs(delta_table_locations) do
+          print("Delta Lake exported table \"" .. t .. "\"'s location: " .. loc .. "\n")
+        end


Same question applies here

itaiad200 · 2024-02-08T16:30:59Z

esti/export_hooks_files/delta/azure/_lakefs_actions/delta_export.yaml

+        end
+      args:
+        azure:
+          storage_account: "{{ .AzureStorageAccount }}"


Is this a secret? Consider just putting this as plaintext here if it isn't

It makes it easier to change the storage account name (in a single place) in the code and not in the yaml file

itaiad200 · 2024-02-08T16:37:21Z

esti/catalog_export_test.go

+	})
+
+	runs := waitForListRepositoryRunsLen(ctx, t, repo, headCommit.Id, 1)
+	require.Equal(t, "completed", runs.Results[0].Status)


Consider checking the run result name or other identifier to make sure you're seeing the correct result

Isan-Rivkin

Hey, that's a big PR so it'll take some time but I added the most meaningful things that I found so far.

Isan-Rivkin · 2024-02-11T08:48:50Z

pkg/actions/lua/storage/azure/client.go

+
+// DeleteRecursive Should be implemented when needed. There are nuances between HNS and BlobStorage which requires understanding the
+// Actual use case before implementing the solution
+func (c *Client) DeleteRecursive(l *lua.State) int {


Let's remove this function, why keep it if it's not implemented? I find it very confusing including docs.

For conformity we created a client interface, the client objects need to implement that interface to support all the required functionalities.
This is relevant beyond the scope of the delta exporter. I think it is essential to keep the code modular.
In the future when we decide to add support for Hive export in Azure - we will need to implement it.

Isan-Rivkin · 2024-02-11T08:55:37Z

pkg/actions/lua/storage/aws/client.go

There is AWS Client for S3 and Glue, why did you rename s3.go to client.go? Also keeping the glue.go
Let's just keep the files separated with clear names like e.g s3.go.

Isan-Rivkin · 2024-02-11T08:59:56Z

pkg/actions/lua/storage/azure/client.go

+	client *service.Client
+}
+
+func newClient(ctx context.Context) lua.Function {


Can you be explicit about that client since it's not general Azure client instead storage client?
We're investing in our lua SDK's not only for export hooks, the azure SDK should be consistent with AWS where we have glue, s3 etc.
Now it's easy to change, later on when we want to add some outside of azure storage functionality this will get confusing very fast.

Isan-Rivkin · 2024-02-11T09:48:53Z

pkg/actions/lua/storage/client.go

+	DeleteRecursive(l *lua.State) int
+}
+
+func InitStorageClient(l *lua.State, client Client) {


IMO we don't need this abstraction, I fear it's too early and make things more complex.

To make it easier to use with exporter like export_delta_log you can use the function param write_object and pass a wrapper function for azure storage client write_object: function(bucket, key, data) it doesn't need to be on the SDK level.

GCS is not included here

In S3/Azure the interface has 2 missing functions (delete_recursive, list_objects missing)

PutObject(l *lua.State) int is not not matching in both cases (referring to implementation with comment // Skipping first argument as it is the host arg (bucket which is irrelevant in Azure) in azure that you added.).

Those SDK's are not meant to be used by export hooks only, our users should be able to use those SDK independently for their own needs, the combination of them here doesn't seem friendly to user experience.

pkg/actions/lua/lakefs/catalogexport/table_extractor.lua

Isan-Rivkin

Thank you for addressing previous comment!
Added some comments, overall looks like a very useful addition :)

pkg/actions/lua/lakefs/catalogexport/table_extractor.lua

docs/howto/hooks/lua.md

pkg/actions/lua/storage/azure/azure.go

Co-authored-by: isan_rivkin <[email protected]>

Isan-Rivkin

LGTM! Can't wait to use it!

N-o-Z added the include-changelog PR description should be included in next release changelog label Feb 6, 2024

N-o-Z self-assigned this Feb 6, 2024

N-o-Z force-pushed the task/delta-exporter-for-azure-7296 branch 10 times, most recently from ec576ce to d2fdb99 Compare February 8, 2024 06:13

Delta Exporter: Azure Support

f85324a

N-o-Z force-pushed the task/delta-exporter-for-azure-7296 branch from d2fdb99 to f85324a Compare February 8, 2024 06:53

N-o-Z requested review from Isan-Rivkin and Jonathan-Rosenberg February 8, 2024 09:27

N-o-Z marked this pull request as ready for review February 8, 2024 09:27

itaiad200 approved these changes Feb 8, 2024

View reviewed changes

Isan-Rivkin requested changes Feb 11, 2024

View reviewed changes

Isan-Rivkin requested changes Feb 12, 2024

View reviewed changes

pkg/actions/lua/lakefs/catalogexport/table_extractor.lua Outdated Show resolved Hide resolved

CR Fixes

672b289

N-o-Z requested a review from Isan-Rivkin February 12, 2024 13:21

Isan-Rivkin requested changes Feb 13, 2024

View reviewed changes

pkg/actions/lua/lakefs/catalogexport/table_extractor.lua Outdated Show resolved Hide resolved

docs/howto/hooks/lua.md Outdated Show resolved Hide resolved

pkg/actions/lua/storage/azure/azure.go Show resolved Hide resolved

N-o-Z and others added 2 commits February 13, 2024 15:52

Update pkg/actions/lua/lakefs/catalogexport/table_extractor.lua

d095399

Co-authored-by: isan_rivkin <[email protected]>

Documentation

1032728

N-o-Z requested a review from Isan-Rivkin February 13, 2024 14:33

Isan-Rivkin approved these changes Feb 13, 2024

View reviewed changes

N-o-Z enabled auto-merge (squash) February 13, 2024 14:40

N-o-Z merged commit ab22684 into master Feb 13, 2024
36 checks passed

N-o-Z deleted the task/delta-exporter-for-azure-7296 branch February 13, 2024 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Delta Exporter: Azure Support #7444

Delta Exporter: Azure Support #7444

N-o-Z commented Feb 6, 2024

github-actions bot commented Feb 6, 2024 •

edited

Loading

github-actions bot commented Feb 6, 2024

itaiad200 left a comment

itaiad200 Feb 8, 2024

itaiad200 Feb 8, 2024

N-o-Z Feb 12, 2024

itaiad200 Feb 8, 2024

N-o-Z Feb 12, 2024

itaiad200 Feb 8, 2024

N-o-Z Feb 12, 2024

itaiad200 Feb 8, 2024

itaiad200 Feb 8, 2024

N-o-Z Feb 12, 2024

itaiad200 Feb 8, 2024

N-o-Z Feb 12, 2024

Isan-Rivkin left a comment

Isan-Rivkin Feb 11, 2024

N-o-Z Feb 11, 2024

Isan-Rivkin Feb 11, 2024

Isan-Rivkin Feb 11, 2024

Isan-Rivkin Feb 11, 2024

Isan-Rivkin left a comment

Isan-Rivkin left a comment

Delta Exporter: Azure Support #7444

Delta Exporter: Azure Support #7444

Conversation

N-o-Z commented Feb 6, 2024

Change Description

Background

Testing Details

Breaking Change?

github-actions bot commented Feb 6, 2024 • edited Loading

github-actions bot commented Feb 6, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

itaiad200 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

Isan-Rivkin 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

Isan-Rivkin left a comment

Choose a reason for hiding this comment

Isan-Rivkin left a comment

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2024 •

edited

Loading