Skip to content

Comments

feat: Add SpawnService and SpawnedReqwestConnector for running requests on a different runtime#332

Merged
alamb merged 13 commits intoapache:mainfrom
ion-elgreco:feat/spawn-service
May 6, 2025
Merged

feat: Add SpawnService and SpawnedReqwestConnector for running requests on a different runtime#332
alamb merged 13 commits intoapache:mainfrom
ion-elgreco:feat/spawn-service

Conversation

@ion-elgreco
Copy link
Contributor

@ion-elgreco ion-elgreco commented Apr 13, 2025 •
edited
Loading

Which issue does this PR close?

Adds support for spawning reqwuests tasks to another Runtime. Credits to @tustvold, I simply added an additional connector in there to make it easier for others to integrate this in their code base.

Example:

  let prefix = Path::parse(path)?;
  let mut builder = MicrosoftAzureBuilder::new().with_url(url.to_string());
  for (key, value) in config.iter() {
      builder = builder.with_config(*key, value.clone());
  }
  let inner = builder
      .with_retry(self.parse_retry_config(options)?)
      .with_http_connector(SpawnedReqwestConnector::new(io_handle))
      .build()?;

Rationale for this change

Allows a separation of cpu bound tasks and io bound tasks

What changes are included in this PR?

  • Adds a SpawnService
  • Adds a SpawnedReqwestConnector

Are there any user-facing changes?

Only an additional http_connector.

cc @alamb 😃

@ion-elgreco ion-elgreco marked this pull request as ready for review April 13, 2025 10:00
@ion-elgreco ion-elgreco mentioned this pull request Apr 13, 2025
Closed
4 tasks
alamb
alamb approved these changes Apr 13, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ion-elgreco (and @tustvold ) -- this is great.

I think it would help a lot of we could add some more documentation / examples

Basically, imagine someone coming to https://docs.rs/object_store/latest/object_store/

  1. How would they find out about this feature / decide when to use it
  2. How would they be able to use it?

Perhaps we could

  1. Add a brief section after TLS Certificiates with a pointer to the src/client/http module
  2. Add some documentation examples in http with pointers to SpawnService

I can try and help with this documentation later this week

Example:

Is there any chance you can add this example somewhere?

@alamb alamb requested a review from tustvold April 13, 2025 12:34
@alamb
Copy link
Contributor

alamb commented Apr 13, 2025

FYI @kylebarron

@ion-elgreco
Copy link
Contributor Author

ion-elgreco commented Apr 13, 2025

@alamb sure! I'll add more docs tomorrow

@ion-elgreco ion-elgreco mentioned this pull request Apr 13, 2025
Merged
@ion-elgreco
Copy link
Contributor Author

ion-elgreco commented Apr 14, 2025

@alamb I've added some docs

@alamb
Copy link
Contributor

alamb commented Apr 15, 2025

Note due to #335 the CI was not running. I plan to merge up from main shortly

alamb
alamb reviewed Apr 15, 2025
View reviewed changes
alamb
alamb reviewed Apr 15, 2025
View reviewed changes
@alamb alamb changed the title feat: spawn-service feat: Add SpawnService and SpawnedReqwestConnector for running requests on a different runtime Apr 15, 2025
alamb
alamb approved these changes Apr 15, 2025
View reviewed changes
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @ion-elgreco 🙏

As I mentioned I spent a while longer today updating the docs and adding some more tests. I think this PR is now ready to rock. Thank you so much

This completes a feature we have been talking about / discussing for over a year. 💯 🦾 !

FYI @crepererum and @tustvold

src/aws/mod.rs

/// Integration test that ensures I/O is done on an alternate threadpool
/// when using the `SpawnedReqwestConnector`.
#[test]
Copy link
Contributor

@alamb alamb Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wrote this end to end integration test to ensure that everything worked correctly when hooked up to an ObjectStore implementation (and it does!)

src/aws/mod.rs

// Runtime with I/O enabled
let io_runtime = tokio::runtime::Builder::new_current_thread()
.enable_all() // <-- turns on IO
Copy link
Contributor

@alamb alamb Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you comment out this line the test fails

@alamb
Copy link
Contributor

alamb commented Apr 17, 2025

I am out starting tomorrow for a week, so if another committer doesn't merge this before I will do so when I get back

Thanks again @ion-elgreco

@tustvold
Copy link
Contributor

tustvold commented Apr 21, 2025

This appears to have accumulated some merge conflicts

@alamb alamb mentioned this pull request Apr 28, 2025
Closed
26 tasks
@alamb
Copy link
Contributor

alamb commented Apr 28, 2025

This appears to have accumulated some merge conflicts

I'll try and fix it

@ion-elgreco
Copy link
Contributor Author

ion-elgreco commented May 1, 2025

@alamb resolved the merge conflict with the new Zed merge conflict functionality :P

@alamb alamb mentioned this pull request May 5, 2025
Closed
23 tasks
@alamb
Copy link
Contributor

alamb commented May 6, 2025

I am looking at the WASM issue

@alamb
Copy link
Contributor

alamb commented May 6, 2025 •
edited
Loading

I think we are ready , let's merge this in!

@alamb alamb merged commit e0625b3 into apache:main May 6, 2025
@alamb
Copy link
Contributor

alamb commented May 6, 2025

Thanks again @ion-elgreco for helping push this over the line

@alamb alamb mentioned this pull request May 13, 2025
Closed
@alamb alamb mentioned this pull request Jun 8, 2025
Merged
@linhr linhr mentioned this pull request Jul 19, 2025
Open
AdamGS pushed a commit to AdamGS/arrow-rs-object-store that referenced this pull request Sep 4, 2025
@ion-elgreco @tustvold
@alamb @AdamGS
feat: Add SpawnService and SpawnedReqwestConnector for running re…
d3a59bb 
…quests on a different runtime (apache#332)

* feat: spawn service

* feat: add SpawnedReqwestConnector

* chore: add zed settings to .gitignore

* chore: add docs

* Tweak main page documentation

* Update documentation and fix example

* Fix ci

* Add end to end integration test

* cleanup test

* use separate bucket for test

* try and fix wasm build

---------

Co-authored-by: Raphael Taylor-Davies <[email protected]>
Co-authored-by: Andrew Lamb <[email protected]>
@alexanderbianchi alexanderbianchi mentioned this pull request Feb 19, 2026
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@tustvold tustvold Awaiting requested review from tustvold

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[object_store] Run requests on a different tokio runtime

3 participants

@ion-elgreco @alamb @tustvold