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

feat: indexer resolution #476

Merged
merged 13 commits into from
Nov 10, 2022
Merged

feat: indexer resolution #476

merged 13 commits into from
Nov 10, 2022

Conversation

b5
Copy link
Member

@b5 b5 commented Nov 8, 2022

supersedes #416. Using a separate PR to keep things comparable. Updating this branch changed a lifetime parameter that has me somewhat concerned.

@b5 b5 requested a review from flub November 8, 2022 05:49
@b5 b5 marked this pull request as ready for review November 8, 2022 15:59
@b5 b5 requested a review from flub November 8, 2022 19:17
@b5 b5 changed the title B5/feat indexer resolution feat: indexer resolution Nov 8, 2022
behaviour is now merged into the FullResolver
@b5 b5 requested a review from rklaehn November 8, 2022 20:43
@b5
Copy link
Member Author

b5 commented Nov 8, 2022

This is largely reviewed-and-vetted (but not well tested) code written by @dignifiedquire that we don't want to let get too stale. Feel free to put as much or as little time into review as necessary. I can provide context on indexers & whatnot if needed

@rklaehn
Copy link
Contributor

rklaehn commented Nov 9, 2022

It is weird for a p2p system to do such things: call gateways and indexer nodes via http. Might as well put the data on s3. But it seems to do what it advertises...

flub
flub previously approved these changes Nov 9, 2022
{
let mut bytes = unsigned_varint::encode::u32_buffer();
unsigned_varint::encode::u32(*t as u32, &mut bytes);
serializer.serialize_str(&base64::encode(&bytes))
Copy link
Contributor

Choose a reason for hiding this comment

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

probably want to make stable clippy happy here and remove the &

rklaehn
rklaehn previously approved these changes Nov 9, 2022
Copy link
Contributor

@rklaehn rklaehn left a comment

Choose a reason for hiding this comment

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

It seems to do what is advertised.

I find it weird, so let's do it for now and try to get rid of it at some point. Not this code - the whole concept of a p2p system using centralised indexer nodes and gateways...

See the comments for some minor improvements.

FullLoaderConfig {
http_gateways: config
.http_resolvers
.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

.iter()
.flatten()

instead of

.clone()
.unwrap_or_default()
.into_iter()

would avoid having to clone things.

let loader_config = FullLoaderConfig {
http_gateways: config
.http_resolvers
.as_ref()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a test, but

.iter()
.flatten()
.map(|u| GatewayUrl::from_str(u).unwrap())

would look less weird.

let loader_config = FullLoaderConfig {
http_gateways: config
.http_resolvers
.as_ref()
Copy link
Contributor

Choose a reason for hiding this comment

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

.iter()
.flatten()

.map(|s| &s[..])
.unwrap_or(&[][..])
.iter()
.map(|u| u.parse().unwrap())
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we collect into an anyhow::Result and do the unwrap outside? If the strings are unparseable, this whole method will panic, right?

Ah, right, this is only used from tests. Then it is OK I guess. Although I usually also use anyhow::Result<()> in tests.

async fn fetch_car_recursive() -> anyhow::Result<()> {

will fail the test if an error is returned.

FullLoaderConfig {
http_gateways: config
.http_resolvers
.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

.iter().flatten()

http_gateways: config
.gateway
.http_resolvers
.clone()
Copy link
Contributor

Choose a reason for hiding this comment

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

.iter().flatten()

@fabricedesre
Copy link
Contributor

It is weird for a p2p system to do such things: call gateways and indexer nodes via http. Might as well put the data on s3. But it seems to do what it advertises...

Probably not the place to discuss this, but anyway: I don't see IPFS primarily as a p2p system, but as a content based addressing stack. The beauty of that is that you can rely on whatever transport and should use the most efficient one in all situations, which p2p often is not...

@b5 b5 dismissed stale reviews from rklaehn and flub via 2e4f39c November 9, 2022 21:24
@b5 b5 requested review from rklaehn and flub November 9, 2022 22:00
@b5 b5 added this to the v0.1.1 milestone Nov 10, 2022
@b5 b5 merged commit f744949 into main Nov 10, 2022
@b5 b5 deleted the b5/feat-indexer-resolution branch November 11, 2022 02:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants