feat(services/azdls): list start from and sas token support #5242

alexwilcoxson-rel · 2024-10-24T20:51:48Z

Which issue does this PR close?

Closes #.

Rationale for this change

This changes proposes implementation of the start_after workaround used by Hadoop to improve listing in ADLS from a particular point.

This is important for table formats like Delta Lake that make use of list_with_offset in object store to get the latest version of a table.

In my testing listing directories with > 10k records similar to what Delta Lake would do improves times from seconds to milliseconds (~3seconds to 60ms).

Also this adds SAS token support which could be split into its own PR but needed it for testing against my company's ADLS accounts.

What changes are included in this PR?

Changes to azdls module to include list with start_after functionality. This includes a module crc64 which computes crc for use in the continuation token generated for start after

Are there any user-facing changes?

Yes, start_after will function for azdls, and SAS token support.

@Xuanwo I'm opening this as a draft to get initial feedback. This is not directly supported by Microsoft, or I should say documented. In pursuit of them adding this capability to the azblob endpoints, they were the ones that directed me to Hadoop example, so I have confidence in it but could see if its something they would officially support/document.

One caveat as well is I believe (per the hadoop code) the change is will only work if hierarchical namespace (xns) is enabled which isn't something I believe we can tell when the list request is being made. It could be the user has to opt in and inform if xns is enabled. Hadoop code does another request to figure that out dynamically which I'd personally want to avoid.

Xuanwo

Hi, thank you @alexwilcoxson-rel for working on this. I've been considering porting logic from Hadoop to OpenDAL for a while but haven't taken action yet.

This feature is undocumented, so it's best not to enable it by default. How about adding a new flag called enable_list_with_start_after, where we set the capability list_with_start_after to true?

Xuanwo · 2024-10-25T01:44:19Z

core/src/services/azdls/backend.rs

@@ -133,6 +133,15 @@ impl AzdlsBuilder {
        self
    }

+    /// Set sas_token of this backend.
+    pub fn sas_token(mut self, sas_token: &str) -> Self {


Reqsign has sas_token support; we can use it here: https://docs.rs/reqsign/0.16.0/reqsign/struct.AzureStorageConfig.html#structfield.sas_token

Maybe it's worth a separate PR?

Are you suggesting we use reqsign::AzureStorageConfig on AzdlsBuilder or AzdlsConfig or something else?

Yes, we can construct like s3 does:

opendal/core/src/services/s3/backend.rs

Lines 745 to 783 in 3fe8caa

// This is our current config.

let mut cfg = AwsConfig::default();

if !self.config.disable_config_load {

#[cfg(not(target_arch = "wasm32"))]

{

cfg = cfg.from_profile();

cfg = cfg.from_env();

}

}

if let Some(v) = self.config.region.clone() {

cfg.region = Some(v);

}

if cfg.region.is_none() {

return Err(Error::new(

ErrorKind::ConfigInvalid,

"region is missing. Please find it by S3::detect_region() or set them in env.",

)

.with_operation("Builder::build")

.with_context("service", Scheme::S3));

}

let region = cfg.region.to_owned().unwrap();

debug!("backend use region: {region}");

// Building endpoint.

let endpoint = self.build_endpoint(&region);

debug!("backend use endpoint: {endpoint}");

// Setting all value from user input if available.

if let Some(v) = self.config.access_key_id {

cfg.access_key_id = Some(v)

}

if let Some(v) = self.config.secret_access_key {

cfg.secret_access_key = Some(v)

}

if let Some(v) = self.config.session_token {

cfg.session_token = Some(v)

}

core/src/services/azdls/backend.rs

core/src/services/azdls/crc64.rs

core/src/services/azdls/core.rs

core/src/services/azdls/crc64.rs

alexwilcoxson-rel · 2024-10-28T20:13:51Z

hey @Xuanwo as I was refactoring the code I wrote a test against one of our actual storage accounts. I then executed the list call with recursive = true as the object_store integration does. However when I do that list result from azure contains everything.

I'm trying to figure out how recursive could be impacting the azdls list but I don't see it used by azdls service.

EDIT: ah I see the CompleteLayer, looking into this.

…t list complete layer

alexwilcoxson-rel · 2024-10-29T01:31:41Z

Ok so I found the FlatLister does not retain the OpList args.

Adding modifying FlatLister to take in the original args and pass them to the inner lister seems to work for this case, but I'm not sure how recursive listing and start_after should interact for all edge cases.

The underlying Azure REST API does have a recursive parameter, but when actually using that:

the recursive listing performance in general is not great on azdls
start_after continuation token causes 500 when recursive = true
so I am hesitant to enable it

Xuanwo · 2024-10-29T02:29:43Z

Thank you @alexwilcoxson-rel for catching this.

Adding modifying FlatLister to take in the original args and pass them to the inner lister seems to work for this case, but I'm not sure how recursive listing and start_after should interact for all edge cases.

It should be fine as long as we handle the capability correctly.

The underlying Azure REST API does have a recursive parameter, but when actually using that:

the recursive listing performance in general is not great on azdls

start_after continuation token causes 500 when recursive = true
so I am hesitant to enable it

That seems to be a problem. Would you like to raise a separate issue for this? We can track it and switch to azdls's own recursive when available.

Xuanwo · 2024-10-29T02:31:53Z

core/src/raw/oio/list/flat_list.rs

@@ -92,7 +94,7 @@ where
    async fn next(&mut self) -> Result<Option<oio::Entry>> {
        loop {
            if let Some(de) = self.next_dir.take() {
-                let (_, l) = self.acc.list(de.path(), OpList::new()).await?;
+                let (_, l) = self.acc.list(de.path(), self.args.clone()).await?;


It's a bit tricky since FlatLister is used to simulate recursive listing on unsupported services. Therefore, we can't forward OpList directly. Instead, let's introduce a start_after here, and FlatLister should ensure the OpList passed to undering are constructed correctly.

meaning OpList::new() with start_after set to a self.start_after value? or am I misinterpreting?

meaning OpList::new() with start_after set to a self.start_after value?

Yep.

Xuanwo · 2024-10-29T02:33:28Z

core/src/services/azdls/backend.rs

@@ -133,6 +133,15 @@ impl AzdlsBuilder {
        self
    }

+    /// Set sas_token of this backend.
+    pub fn sas_token(mut self, sas_token: &str) -> Self {


Yes, we can construct like s3 does:

opendal/core/src/services/s3/backend.rs

Lines 745 to 783 in 3fe8caa

// This is our current config.

let mut cfg = AwsConfig::default();

if !self.config.disable_config_load {

#[cfg(not(target_arch = "wasm32"))]

{

cfg = cfg.from_profile();

cfg = cfg.from_env();

}

}

if let Some(v) = self.config.region.clone() {

cfg.region = Some(v);

}

if cfg.region.is_none() {

return Err(Error::new(

ErrorKind::ConfigInvalid,

"region is missing. Please find it by S3::detect_region() or set them in env.",

)

.with_operation("Builder::build")

.with_context("service", Scheme::S3));

}

let region = cfg.region.to_owned().unwrap();

debug!("backend use region: {region}");

// Building endpoint.

let endpoint = self.build_endpoint(&region);

debug!("backend use endpoint: {endpoint}");

// Setting all value from user input if available.

if let Some(v) = self.config.access_key_id {

cfg.access_key_id = Some(v)

}

if let Some(v) = self.config.secret_access_key {

cfg.secret_access_key = Some(v)

}

if let Some(v) = self.config.session_token {

cfg.session_token = Some(v)

}

alexwilcoxson-rel · 2024-10-30T00:55:32Z

The underlying Azure REST API does have a recursive parameter, but when actually using that:

the recursive listing performance in general is not great on azdls

start_after continuation token causes 500 when recursive = true
so I am hesitant to enable it

That seems to be a problem. Would you like to raise a separate issue for this? We can track it and switch to azdls's own recursive when available.

Yes I will create a follow up issue to switch to use recursive parameter available on azdls list. Mentioning though the seeming incompatibility with this start_after workaround

alexwilcoxson-rel · 2024-10-30T00:56:50Z

This feature is undocumented, so it's best not to enable it by default. How about adding a new flag called enable_list_with_start_after, where we set the capability list_with_start_after to true?

gate behind feature

alexwilcoxson-rel · 2024-11-04T19:22:09Z

Hey @Xuanwo just letting you know I probably won't get back to this until later in the week. I will share though that I am using this in our fork of version ~0.48 I think. We're using it along size the base object_store azure store (since those have all the put support for delta), and it has gone well!

Looking forward to getting this in along with the put if-not-match changes that are in progress as well!

feat(services/azdls): list start from and sas token support

ddfd3ae

github-actions bot added the releases-note/feat label Oct 24, 2024

alexwilcoxson-rel mentioned this pull request Oct 24, 2024

Use reconstructed ListBlobs marker to provide list offset support in MicrosoftAzure store apache/arrow-rs#6174

Draft

chore(services/azdls): clippy

384c71e

Xuanwo reviewed Oct 25, 2024

View reviewed changes

Xuanwo mentioned this pull request Oct 25, 2024

Oct 21, 2024: This week in OpenDAL #5220

Closed

alexwilcoxson-rel added 2 commits October 28, 2024 20:29

feat(services-azdls): refactored crc64 and pass list args through fla…

aba400e

…t list complete layer

feat(services-azdls): fmt

c370249

Merge branch 'main' into azdls-list-start-from

df6cf90

Xuanwo reviewed Oct 29, 2024

View reviewed changes

Xuanwo mentioned this pull request Oct 29, 2024

Oct 28, 2024: This week in OpenDAL #5251

Closed

Merge branch 'main' into azdls-list-start-from

7be7756

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(services/azdls): list start from and sas token support #5242

feat(services/azdls): list start from and sas token support #5242

alexwilcoxson-rel commented Oct 24, 2024

Xuanwo left a comment

Xuanwo Oct 25, 2024

alexwilcoxson-rel Oct 26, 2024

Xuanwo Oct 29, 2024

alexwilcoxson-rel commented Oct 28, 2024 •

edited

Loading

alexwilcoxson-rel commented Oct 29, 2024

Xuanwo commented Oct 29, 2024

Xuanwo Oct 29, 2024

alexwilcoxson-rel Oct 30, 2024

Xuanwo Oct 30, 2024

Xuanwo Oct 29, 2024

alexwilcoxson-rel commented Oct 30, 2024

alexwilcoxson-rel commented Oct 30, 2024

alexwilcoxson-rel commented Nov 4, 2024

	// This is our current config.
	let mut cfg = AwsConfig::default();
	if !self.config.disable_config_load {
	#[cfg(not(target_arch = "wasm32"))]
	{
	cfg = cfg.from_profile();
	cfg = cfg.from_env();
	}
	}

	if let Some(v) = self.config.region.clone() {
	cfg.region = Some(v);
	}
	if cfg.region.is_none() {
	return Err(Error::new(
	ErrorKind::ConfigInvalid,
	"region is missing. Please find it by S3::detect_region() or set them in env.",
	)
	.with_operation("Builder::build")
	.with_context("service", Scheme::S3));
	}

	let region = cfg.region.to_owned().unwrap();
	debug!("backend use region: {region}");

	// Building endpoint.
	let endpoint = self.build_endpoint(&region);
	debug!("backend use endpoint: {endpoint}");

	// Setting all value from user input if available.
	if let Some(v) = self.config.access_key_id {
	cfg.access_key_id = Some(v)
	}
	if let Some(v) = self.config.secret_access_key {
	cfg.secret_access_key = Some(v)
	}
	if let Some(v) = self.config.session_token {
	cfg.session_token = Some(v)
	}

feat(services/azdls): list start from and sas token support #5242

Are you sure you want to change the base?

feat(services/azdls): list start from and sas token support #5242

Conversation

alexwilcoxson-rel commented Oct 24, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Xuanwo left a comment

Choose a reason for hiding this comment

Xuanwo Oct 25, 2024

Choose a reason for hiding this comment

alexwilcoxson-rel Oct 26, 2024

Choose a reason for hiding this comment

Xuanwo Oct 29, 2024

Choose a reason for hiding this comment

alexwilcoxson-rel commented Oct 28, 2024 • edited Loading

alexwilcoxson-rel commented Oct 29, 2024

Xuanwo commented Oct 29, 2024

Xuanwo Oct 29, 2024

Choose a reason for hiding this comment

alexwilcoxson-rel Oct 30, 2024

Choose a reason for hiding this comment

Xuanwo Oct 30, 2024

Choose a reason for hiding this comment

Xuanwo Oct 29, 2024

Choose a reason for hiding this comment

alexwilcoxson-rel commented Oct 30, 2024

alexwilcoxson-rel commented Oct 30, 2024

alexwilcoxson-rel commented Nov 4, 2024

alexwilcoxson-rel commented Oct 28, 2024 •

edited

Loading