add support for for datalake api #1893

mahmoudbahaa · 2023-04-01T10:57:21Z

The PR was externally tested (through java unit tests that use Hadoop File System and the abfs:// scheme and Azure sdk)

Didn't have enough time to add test cases to the repo itself. May add some in the future if I had the time. or if someone want to do that go ahead.

Address issue #553

mahmoudbahaa · 2023-04-01T10:58:59Z

@microsoft-github-policy-service agree company="Incorta"

mahmoudbahaa · 2023-04-01T23:30:12Z

fix existing test cases by merging different conflicting apis into one and reset enable xml for all except listpaths (done by manual change in generated code by changing isXML of the opration to false)

blueww · 2023-04-03T08:47:53Z

@mahmoudbahaa

Thanks so much for raising this PR!
It's so great to see the PR for datalake gen2 support.

However, I have a sanity check on it, and see some problems need fix:

The PR doesn't introduce a new endpoint dfs, all dfs requests are handle by blob endpoint.
This will take problem:

It will cause 2 blob/dfs request which has simialr parameter like Path_GetProperties and Blob_GetProperties will handled by same code, but actually in rest spec they have different parameter and responds.
This will take regression to exist blob API, so exist Azurite user on blob API might be blocked.
This will make Azurite not scalable for coming blob/dfs API change, or need much effort to avoid the regression on each endpoint.

So to make Azurite works like product azure, we should introduce a new endpoint dfs.

And some Blob endpoint API will work differently on HNS enabled account and not enabled account. So to support both account type, we might should add a new start parameter to Azurite to indicate the account enabled HNS or not. (To avoid regression, default should be not enabled.)
Please write doc for which features/API has already supported, and which has not, this is very important for customer to using the feature in Azurite, and for the following improvement/trouble shooting. You can refer https://github.com/Azure/Azurite#support-matrix for how to write it.
As I see, there might still be some features/API not implemented or handled correctly in the PR. include but not limited to:

Path_SetAccessControl: not implemented
Datalake SAS not work (include "sr=d","sdd=")
Path_Create : header "x-ms-permissions" and "x-ms-umask" works not well (with "x-ms-permissions: rwxrwxrwx" and "x-ms-umask: 0070", Public Azure will get "rwx---rwx", but current Azurite get "rwxrwxr--")
FileSystem_ListPaths/Path_GetProperties:
- list from filesystem root failed with 404
- Result : missing many important properties like permission/group, FileSystem_ListPaths has additional "isDirectory":false which will cause .net SDK fail,

We don't allow manually update auto-generated code, or generate code again with new swagger will overwrite the manual change. The correct way is to update swagger and record the changes in Readme.md (better with directive), or update the generate code tool.
As you have mentioned, test case still not added. But enough test coverage is necessary to merge the PR, to ensure quanlity and avoid regression.
Missing some necessary doc changes like Readme, change log ...

mahmoudbahaa · 2023-04-10T11:40:04Z

@blueww

reagrding your commments:

The PR doesn't introduce a new endpoint dfs, all dfs requests are handle by blob endpoint.

I introduced new endpoint named dfs on port 10003 which support both blob and dfs requests.

It will cause 2 blob/dfs request which has simialr parameter like Path_GetProperties and Blob_GetProperties will handled by same code, but actually in rest spec they have different parameter and responds.

I merged both parameters and responds in these cases into the dfs one and removed the blob one

This will take regression to exist blob API, so exist Azurite user on blob API might be blocked.

Changes made to blob endpoint are now minimal

And some Blob endpoint API will work differently on HNS enabled account and not enabled account. So to support both account type, we might should add a new start parameter to Azurite to indicate the account enabled HNS or not. (To avoid regression, default should be not enabled.)

for now the HNS is enabled by default in dfs endpoint since some endpoints don't work for non-HNS enabled and in datalake gen2 it makes sense to make HNS enabled by default.

Please write doc for which features/API has already supported, and which has not, this is very important for customer to using the feature in Azurite, and for the following improvement/trouble shooting. You can refer https://github.com/Azure/Azurite#support-matrix for how to write it.

done

Path_SetAccessControl: not implemented

done but the recursive one is not supported yet as in the support matrix

Datalake SAS not work (include "sr=d","sdd=")

should be working now

Path_Create : header "x-ms-permissions" and "x-ms-umask" works not well (with "x-ms-permissions: rwxrwxrwx" and "x-ms-umask: 0070", Public Azure will get "rwx---rwx", but current Azurite get "rwxrwxr--")

should be working properly now

FileSystem_ListPaths/Path_GetProperties:
list from filesystem root failed with 404
Result : missing many important properties like permission/group,

permission/group is now included in both

FileSystem_ListPaths has additional "isDirectory":false which will cause .net SDK fail,

actually according to spec in listPaths each Path should have have isDirectory flag https://learn.microsoft.com/en-us/rest/api/storageservices/datalakestoragegen2/path/list#path

We don't allow manually update auto-generated code, or generate code again with new swagger will overwrite the manual change. The correct way is to update swagger and record the changes in Readme.md (better with directive), or update the generate code tool.

I did almost all needed changes to swagger and documented in dfs.md file that generate the swagger. only remaining changes needed that is not included in the swagger but are also documented in dfs.md:

isXML is set to false in listPaths (generator should respect accept property of the request and serialize according to it) [needed for hadoop]
add req.getHeader("X-HTTP-Method-Override") in dispatch.middleware.ts since it can be used instead of req.getHeader("X-HTTP-Method") sometimes [also needed for hadoop]

as far as i can tell the actual generator code is private so don't have access to it to change those

As you have mentioned, test case still not added. But enough test coverage is necessary to merge the PR, to ensure quanlity and avoid regression.

test cases was added specifically for datalake under tests/dfs/storage-file-datalake

Missing some necessary doc changes like Readme, change log ...

added needed changes for readme

please check if other changes is needed

src/dfs/authentication/AccountSASAuthenticator.ts

+    sasProtocol: string = "https,http",
+    requestProtocol: string
+  ): boolean {
+    if (sasProtocol.includes(",")) {


src/dfs/authentication/BlobSASAuthenticator.ts

+    sasProtocol: string = "https,http",
+    requestProtocol: string
+  ): boolean {
+    if (sasProtocol.includes(",")) {


src/dfs/DataLakeRequestListenerFactory.ts

+      authenticationMiddlewareFactory.createAuthenticationMiddleware(
+        authenticators
+      )


src/dfs/context/DataLakeContext.ts

+    return this.context.account;
+  }
+
+  public set account(account: string | undefined) {


blueww · 2023-04-11T06:52:54Z

@mahmoudbahaa

Thanks for the update!

As this is a big PR we might need more time to review it and will reply you later.

For the "isDirectory":false, when list path from server it won't return for file. See following responds from product server in my test.
And one important thing is return it for file will make .net sdk fail with error "The requested operation requires an element of type 'String', but the target element has type 'False'."

{"paths":[
{"contentLength":"0","creationTime":"133256687100169757","etag":"0x8DB3A575D040E1D","expiryTime":"0","group":"$superuser","isDirectory":"true","lastModified":"Tue, 11 Apr 2023 06:38:30 GMT","name":"dir1","owner":"$superuser","permissions":"rwx---rwx"},
{"contentLength":"1024","creationTime":"133256687147652690","etag":"0x8DB3A5760706833","expiryTime":"0","group":"$superuser","lastModified":"Tue, 11 Apr 2023 06:38:35 GMT","name":"dir1/__datetime=2020-09-14 09%3A00%3A00","owner":"$superuser","permissions":"rwx---rwx"},
{"contentLength":"1024","creationTime":"133256687195188636","etag":"0x8DB3A5762ADED9C","expiryTime":"0","group":"$superuser","lastModified":"Tue, 11 Apr 2023 06:38:39 GMT","name":"dir1/f%3Aile2","owner":"$superuser","permissions":"rw-r-----"},
{"contentLength":"1024","creationTime":"133256687207598063","etag":"0x8DB3A57636B47EF","expiryTime":"0","group":"$superuser","lastModified":"Tue, 11 Apr 2023 06:38:40 GMT","name":"dir1/testfile_1K_0","owner":"$superuser","permissions":"rw-r-----"},{"contentLength":"0","creationTime":"133256687133364901","etag":"0x8DB3A575EFE92A5","expiryTime":"0","group":"$superuser","isDirectory":"true","lastModified":"Tue, 11 Apr 2023 06:38:33 GMT","name":"dir2","owner":"$superuser","permissions":"rwxr-x---"}]}

* Full support for sql for the same features supported by Loki except for PageBlob * Added clearDb option to configurations in both blob and dfs and set it to true in the test cases * Added support for using postgreSQL in dfs endpoint (needed due to limitations in mysql) without breaking mysql * Added support for permissions save/load, acl, setExpiry, owner and group * Updated visual studio extension file and added binary azurite-datalake,... * Updated readme, swagger definition changes * Updated change log * Added dfs test pipelines to azure-pipelines

mahmoudbahaa · 2023-04-16T00:31:16Z

@blueww

rebased the PR
squashed the commits
addressed the isDirectory issue by setting it to undefined if it is false (since the default is false as per the specs)

mahmoudbahaa · 2023-04-22T02:54:03Z

@blueww I restructed the PR to reduce duplicate code between blob and dfs end points and splitted it into 2 phases

phase 1 minimal refactor for dfs sake + full support for sql #1933
phase 2 Data lake storage support phase2 #1934

mahmoudbahaa force-pushed the dataLakeStorage-support branch 3 times, most recently from b221b68 to 6283ad0 Compare April 10, 2023 11:22

mahmoudbahaa force-pushed the dataLakeStorage-support branch 2 times, most recently from 48c797f to 71ab093 Compare April 11, 2023 06:42

github-advanced-security bot found potential problems Apr 11, 2023

View reviewed changes

mahmoudbahaa force-pushed the dataLakeStorage-support branch from 12f4ce9 to 6999337 Compare April 15, 2023 23:32

mahmoudbahaa force-pushed the dataLakeStorage-support branch from 2c699e3 to 6d78471 Compare April 16, 2023 00:13

mahmoudbahaa changed the title ~~add support for for datalake api and implemented some methods in it in both Loki and SQL~~ add support for for datalake api Apr 16, 2023

mahmoudbahaa closed this Apr 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for for datalake api #1893

add support for for datalake api #1893

Uh oh!

mahmoudbahaa commented Apr 1, 2023 •

edited

Loading

Uh oh!

mahmoudbahaa commented Apr 1, 2023

Uh oh!

mahmoudbahaa commented Apr 1, 2023

Uh oh!

blueww commented Apr 3, 2023 •

edited

Loading

Uh oh!

mahmoudbahaa commented Apr 10, 2023

Uh oh!

Check failure

Check failure

Check failure

Check failure

blueww commented Apr 11, 2023

Uh oh!

mahmoudbahaa commented Apr 16, 2023

Uh oh!

mahmoudbahaa commented Apr 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

add support for for datalake api #1893

add support for for datalake api #1893

Uh oh!

Conversation

mahmoudbahaa commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahmoudbahaa commented Apr 1, 2023

Uh oh!

mahmoudbahaa commented Apr 1, 2023

Uh oh!

blueww commented Apr 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mahmoudbahaa commented Apr 10, 2023

Uh oh!

Check failure

Check failure

Check failure

Check failure

blueww commented Apr 11, 2023

Uh oh!

mahmoudbahaa commented Apr 16, 2023

Uh oh!

mahmoudbahaa commented Apr 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mahmoudbahaa commented Apr 1, 2023 •

edited

Loading

blueww commented Apr 3, 2023 •

edited

Loading