search

CoffeaTeam / fsspec-xrootd

An XRootD implementation for fsspec
https://coffeateam.github.io/fsspec-xrootd/
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

feat: Switch file reading to use concrete endpoints #48

Closed rpsimeon34 closed 5 months ago

rpsimeon34 commented 9 months ago

Addressing Issue #36

This is not really a multi-source reading commit, but it uses the Pepper-inspired algorithm to find concrete endpoints and read from one of those, rather than through the redirector.

The XRootDFile class gets a list of hosts now, so that could become the basis for a more multi-source approach.

lgray commented 9 months ago

@nsmith- is there anything else this one needs? The use case is becoming ever more pressing.

lgray commented 8 months ago

It might be useful, since this is a bit the "manual transmission" version of the complete fix, to allow a user to specify a list of sites/servers that they will accept as valid endpoint responses. This way we can manage sites that respond immediately which are known to have subpar QoS.

lgray commented 8 months ago

@rpsimeon34 could you please follow up on the review comments.

rpsimeon34 commented 8 months ago

Sorry for the delay. I'll try to implement the locate_all_sources flag and the user-defined valid endpoints list over the weekend. Then I can try some basic performance comparisons after that.

rpsimeon34 commented 8 months ago

I think this should be ready for another round of review when maintainers have time. I'm not sure how to faithfully replicate an XRootD redirector, so I haven't written a proper unit test for the locate_all_sources or valid_sources behaviors. But, some manual testing with files I'm familiar with suggests they work as intended.

P.S. Sorry @lgray for removing and reinstating your logging.debug commit - I thought pre-commit had somehow switched it for me!

lgray commented 8 months ago

No worries, I figured you wanted those print statements in but obviously it's there just for information if you want to see it, hence debug logging.

nsmith- commented 6 months ago

You'll also need to resolve the conflicts since there were a few upstream PRs merged.

rpsimeon34 commented 6 months ago

I believe requests have been addressed. If I misunderstood/mis-implemented something, please let me know and I'm happy to make more changes. Thanks for the review!