Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass Options to HttpBuilder in parse_url_opts #5310

Closed
CarlKCarlK opened this issue Jan 19, 2024 · 5 comments · Fixed by #5311 or #5316
Closed

Pass Options to HttpBuilder in parse_url_opts #5310

CarlKCarlK opened this issue Jan 19, 2024 · 5 comments · Fixed by #5311 or #5316
Labels
enhancement Any new improvement worthy of a entry in the changelog object-store Object Store Interface

Comments

@CarlKCarlK
Copy link

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

Summary

I am trying to use parse_url_opts to create an ObjectPath and StorePath completely from strings (and lists of string pairs). This will make it much easier for my users to access genomic data from the cloud. I can't, however, set the needed timeout option.

Details

I love parse_url_opts. It lets me set options such as "aws_access_key_id".

    let options = [
        ("aws_region", "us-west-2"),
        ("aws_access_key_id", credentials.aws_access_key_id()),
        ("aws_secret_access_key", credentials.aws_secret_access_key()),
    ];

It (apparently) does not, however, let me set client options such as "timeout". This is especially important for
Http.

Describe the solution you'd like

Perhaps the client options could be allowed and prefixed with "client_", for example,

let options = [
    ("client_time_out", "1000"),
];

Aside: I don't know how to set the default units for a duration.

Describe alternatives you've considered
Additional context

@CarlKCarlK CarlKCarlK added the enhancement Any new improvement worthy of a entry in the changelog label Jan 19, 2024
@CarlKCarlK CarlKCarlK changed the title Enhance parse_url_opts to work with client options such as time out. Enhance parse_url_opts to work with client options such as timeout. Jan 19, 2024
@tustvold
Copy link
Contributor

tustvold commented Jan 19, 2024

This could definitely be documented better but it should work to specify timeout

https://docs.rs/object_store/latest/src/object_store/aws/builder.rs.html#381

Values are parsed by https://docs.rs/humantime/latest/humantime/fn.parse_duration.html

@CarlKCarlK
Copy link
Author

CarlKCarlK commented Jan 19, 2024

Thanks for looking at this. I'm wondering if it works with Http?

I think this is the relevant object_store code--It seems all options are ignored in the Http case

https://docs.rs/object_store/latest/src/object_store/parse.rs.html#136-180

        #[cfg(feature = "http")]
        ObjectStoreScheme::Http => {
            let url = &url[..url::Position::BeforePath];
            Box::new(crate::http::HttpBuilder::new().with_url(url).build()?) as _
        }

(I also tried it with Http and it didn't seem to work.)

@tustvold
Copy link
Contributor

Aah yes, it would appear #4516 did not update parse_url. Will get a fix up for you

@tustvold tustvold changed the title Enhance parse_url_opts to work with client options such as timeout. Pass Options to HttpBuilder in parse_url_with_opts Jan 19, 2024
@tustvold tustvold changed the title Pass Options to HttpBuilder in parse_url_with_opts Pass Options to HttpBuilder in parse_url_opts Jan 19, 2024
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jan 19, 2024
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jan 19, 2024
@CarlKCarlK
Copy link
Author

CarlKCarlK commented Jan 19, 2024

[Also added to the PR]

You need to add line 167 back in or this fix doesn't work.
Here is the missing line:

        let url = &url[..url::Position::BeforePath];

CarlKCarlK added a commit to fastlmm/bed-reader that referenced this issue Jan 20, 2024
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jan 20, 2024
tustvold added a commit to tustvold/arrow-rs that referenced this issue Jan 20, 2024
@alamb alamb reopened this Jan 20, 2024
tustvold added a commit that referenced this issue Jan 20, 2024
* Test parse_url_opts for HTTP (#5310)

* Format
@tustvold tustvold added the object-store Object Store Interface label Mar 1, 2024
@tustvold
Copy link
Contributor

tustvold commented Mar 1, 2024

label_issue.py automatically added labels {'object-store'} from #5316

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Any new improvement worthy of a entry in the changelog object-store Object Store Interface
Projects
None yet
3 participants