refactor: ABFS implementation #11419

majetideepak · 2024-11-04T13:54:57Z

Combine AbfsAccount and AbfsConfig in a separate file.
Clean up API naming and clarify semantics.
Add a new constructor for AbfsWriteFile to specify a client. This is used for testing.

netlify · 2024-11-04T13:55:14Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`5475558`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/673625f6696151000840c72a

FelixYBW · 2024-11-04T23:36:08Z

@zhli1142015

zhli1142015 · 2024-11-05T00:12:19Z

velox/connectors/hive/storage_adapters/abfs/AdlsFileClient.h

+ * To facilitate unit testing of file write scenarios, we define the
+ * AzureDatalakeFileClient here, which can be mocked during testing.
+ */
+class AdlsFileClient {


Please use AzureDatalakeFileClient, AdlsFileClient indicates a different thing in our context.
Thanks.

The Azure client name for writing is DataLakeFileClient. I renamed the implementations accordingly and tried to keep the name short.

zhli1142015 · 2024-11-05T00:13:52Z

velox/connectors/hive/storage_adapters/abfs/tests/MockAdlsFileClient.h

+ * https://github.com/Azure/Azurite/wiki/ADLS-Gen2-Implementation-Guidance
+ *
+ * To facilitate unit testing of file write scenarios, we define the
+ * IBlobStorageFileClient here, which can be mocked during testing.


Please update the name here also.

zhli1142015 · 2024-11-05T00:35:01Z

velox/connectors/hive/storage_adapters/abfs/AbfsConfig.h

+
+namespace facebook::velox::filesystems {
+
+static std::string kAzureBlobEndpoint{"fs.azure.blob-endpoint"};


What does this property do? Is this for testing only, if yes please add comments.
Thanks.

majetideepak · 2024-11-05T21:07:55Z

@zhli1142015 thanks for your review! I addressed your comments. Can you take another look?

majetideepak · 2024-11-05T21:23:28Z

@zhli1142015 I noticed that the usage of DataLakeFileClient::Flush is not optimal. We flush when the file is closed
and this might consume a lot of memory if the file written is big. We need to flush similarly to S3 periodically (10Mib chunks). What do you think?

czentgr

nice refactor. Looks much more clean now.

czentgr · 2024-11-05T22:56:45Z

velox/connectors/hive/storage_adapters/abfs/AbfsConfig.cpp

+  std::string_view file;
+  bool isHttps = true;
+  if (path.find(kAbfssScheme) == 0) {
+    file = path.substr(8);


Lets use

file = path.substr(kAbfssScheme.length());

czentgr · 2024-11-05T22:56:56Z

velox/connectors/hive/storage_adapters/abfs/AbfsConfig.cpp

+  if (path.find(kAbfssScheme) == 0) {
+    file = path.substr(8);
+  } else if (path.find(kAbfsScheme) == 0) {
+    file = path.substr(7);


Lets use

file = path.substr(kAbfsScheme.length());

czentgr · 2024-11-06T03:52:46Z

velox/connectors/hive/storage_adapters/abfs/AbfsReadFile.h


-namespace facebook::velox::filesystems::abfs {
+namespace facebook::velox::config {


Not part of your changes but this header doesn't have #pragma once. I think we always should have that?

czentgr · 2024-11-06T04:22:01Z

velox/connectors/hive/storage_adapters/abfs/AbfsWriteFile.cpp

+    const config::ConfigBase& config) {
+  auto abfsAccount = AbfsConfig(path, config);
+  std::shared_ptr<AzureDataLakeFileClient> client =
+      std::make_shared<DataLakeFileClientWrapper>(


Just curious why the DataLakeFileClientWrapper is shared_ptr and not unique_ptr? Would something else access this if it is part of the client?

shared_ptr made it easier to write tests. But I changed this to a unique_ptr as I think it should be as well.

czentgr · 2024-11-06T04:27:04Z

velox/connectors/hive/storage_adapters/abfs/tests/AbfsCommonTest.cpp

+      abfssConfig.connectionString(),
+      "DefaultEndpointsProtocol=https;AccountName=foobar;AccountKey=456;EndpointSuffix=core.windows.net;");
+
+  // test with special characters


Nit. Commit not complete sentence.

czentgr · 2024-11-06T04:36:53Z

velox/connectors/hive/storage_adapters/abfs/tests/AzuriteServer.cpp

+      {{"fs.azure.account.key.test.dfs.core.windows.net", key_},
+       {kAzureBlobEndpoint, endpoint}});
+
+  // Update the default config map with the supplied configOverride map


Nit. Missing .. Or do we even need this comment?

I updated the function documentation instead.

czentgr · 2024-11-06T04:40:40Z

velox/connectors/hive/storage_adapters/abfs/tests/AzuriteServer.h


  virtual ~AzuriteServer();

 private:
  int64_t port_;
+  std::string account_{"test"};


Maybe all of these 4 new members are const?

zhli1142015 · 2024-11-06T06:11:42Z

ient::Flush is not optimal. We flush when the file is closed
and this might consume a lot of memory if the file written is big. We need to flush similarly to S3 periodically (10Mib chunks). What do you think?

I thought we don't cache data and send by chunk. The append API sends data to remote directly.
The behavior you mentioned in S3, may be better, we can take a look later.
Thanks.

majetideepak · 2024-11-06T14:11:23Z

@czentgr thanks for the review. I addressed your comments.

majetideepak · 2024-11-06T19:29:32Z

Filed #11456 for the write improvements.

czentgr

Looks good. Just one nit.

czentgr · 2024-11-07T22:48:36Z

velox/connectors/hive/storage_adapters/abfs/tests/MockDataLakeFileClient.h

    filePath_ = tempFile->getPath();
  }

+  MockDataLakeFileClient(std::string_view filePath) : filePath_(filePath) {}
+
+  std::string_view path() {


Nit, this could be std::string_view path() const.

kevinwilfong · 2024-11-11T17:56:40Z

This needs a maintainer to approve, cc: @Yuhta @xiaoxmeng

https://velox-lib.io/docs/community/components-and-maintainers/

majetideepak · 2024-11-13T12:00:43Z

@kevinwilfong I am the maintainer for the storage_adapters and I approve this :).

kevinwilfong · 2024-11-13T23:32:18Z

I just double checked with the PLC and it sounds like that's not sufficient.

majetideepak · 2024-11-14T16:32:24Z

@zhli1142015 Can you please take a look and approve this PR?

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 4, 2024

majetideepak force-pushed the abfs-gcs-multifs branch 3 times, most recently from 58c4444 to be9b010 Compare November 4, 2024 16:12

zhli1142015 reviewed Nov 5, 2024

View reviewed changes

majetideepak marked this pull request as ready for review November 5, 2024 20:59

majetideepak force-pushed the abfs-gcs-multifs branch from 890d571 to cd4a59f Compare November 5, 2024 21:06

majetideepak force-pushed the abfs-gcs-multifs branch from cd4a59f to 9c88379 Compare November 5, 2024 21:12

czentgr reviewed Nov 6, 2024

View reviewed changes

czentgr approved these changes Nov 7, 2024

View reviewed changes

majetideepak force-pushed the abfs-gcs-multifs branch from 3502540 to 48ea8d7 Compare November 8, 2024 22:58

majetideepak added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label Nov 8, 2024

majetideepak added 2 commits November 14, 2024 11:30

refactor: ABFS implementation

dad9603

fix rebase conflict

eee3afa

majetideepak force-pushed the abfs-gcs-multifs branch from 48ea8d7 to 5475558 Compare November 14, 2024 16:31

majetideepak added 4 commits November 14, 2024 11:31

fix test

a986331

address review comments

f016c1a

address review comments

a84bab5

address review comments

5475558

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: ABFS implementation #11419

refactor: ABFS implementation #11419

majetideepak commented Nov 4, 2024 •

edited

Loading

netlify bot commented Nov 4, 2024 •

edited

Loading

FelixYBW commented Nov 4, 2024

zhli1142015 Nov 5, 2024

majetideepak Nov 5, 2024 •

edited

Loading

zhli1142015 Nov 5, 2024

zhli1142015 Nov 5, 2024

majetideepak commented Nov 5, 2024

majetideepak commented Nov 5, 2024

czentgr left a comment •

edited

Loading

czentgr Nov 5, 2024

czentgr Nov 5, 2024

czentgr Nov 6, 2024

czentgr Nov 6, 2024

majetideepak Nov 6, 2024

czentgr Nov 6, 2024

czentgr Nov 6, 2024

majetideepak Nov 6, 2024

czentgr Nov 6, 2024

zhli1142015 commented Nov 6, 2024

majetideepak commented Nov 6, 2024

majetideepak commented Nov 6, 2024

czentgr left a comment

czentgr Nov 7, 2024

majetideepak Nov 8, 2024

kevinwilfong commented Nov 11, 2024

majetideepak commented Nov 13, 2024

kevinwilfong commented Nov 13, 2024

majetideepak commented Nov 14, 2024


		namespace facebook::velox::filesystems {

		static std::string kAzureBlobEndpoint{"fs.azure.blob-endpoint"};


		namespace facebook::velox::filesystems::abfs {
		namespace facebook::velox::config {

refactor: ABFS implementation #11419

Are you sure you want to change the base?

refactor: ABFS implementation #11419

Conversation

majetideepak commented Nov 4, 2024 • edited Loading

netlify bot commented Nov 4, 2024 • edited Loading

✅ Deploy Preview for meta-velox canceled.

FelixYBW commented Nov 4, 2024

Choose a reason for hiding this comment

majetideepak Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majetideepak commented Nov 5, 2024

majetideepak commented Nov 5, 2024

czentgr left a comment • edited Loading

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

zhli1142015 commented Nov 6, 2024

majetideepak commented Nov 6, 2024

majetideepak commented Nov 6, 2024

czentgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinwilfong commented Nov 11, 2024

majetideepak commented Nov 13, 2024

kevinwilfong commented Nov 13, 2024

majetideepak commented Nov 14, 2024

majetideepak commented Nov 4, 2024 •

edited

Loading

netlify bot commented Nov 4, 2024 •

edited

Loading

majetideepak Nov 5, 2024 •

edited

Loading

czentgr left a comment •

edited

Loading