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

Piece getter for DSN sync #2503

Merged
merged 3 commits into from
Feb 5, 2024
Merged

Piece getter for DSN sync #2503

merged 3 commits into from
Feb 5, 2024

Conversation

nazar-pc
Copy link
Member

@nazar-pc nazar-pc commented Feb 4, 2024

This introduces a new trait for getting pieces for the purposes of DSN sync. The idea behind this change is such that Pulsar and Space Acres can take advantage of piece cache and plots during DSN sync rather than pulling already retrieved pieces from the network again.

This is especially useful because DSN sync and piece cache sync happen in parallel and with #2399 cache will become even bigger. We can leverage all that to make sync experience better.

Code contributor checklist:

ParthDesai
ParthDesai previously approved these changes Feb 5, 2024
Copy link
Contributor

@ParthDesai ParthDesai left a comment

Choose a reason for hiding this comment

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

LGTM. This will be helpful for pulsar.

@ParthDesai
Copy link
Contributor

@nazar-pc Why the base is the force authoring PR's branch? Are these two related?

@nazar-pc
Copy link
Member Author

nazar-pc commented Feb 5, 2024

Not related, I just pushed CI fix into that branch, so this is why

Base automatically changed from require-force-authoring-for-genesis-farmer to main February 5, 2024 06:18
@nazar-pc nazar-pc dismissed ParthDesai’s stale review February 5, 2024 06:18

The base branch was changed.

@shamil-gadelshin
Copy link
Contributor

It's the second time we introduce PieceGetter trait along with its duplicate for the node. Do you think we're ready to create subspace-networking-primitives?

@nazar-pc
Copy link
Member Author

nazar-pc commented Feb 5, 2024

It's the second time we introduce PieceGetter trait along with its duplicate for the node. Do you think we're ready to create subspace-networking-primitives?

This is a different PieceGetter with a different API and goal in mind. For example farmer's getter will reach out to archival storage and node's will only use L2 cache. So I don't see them as duplicates, they serve different purposes. At least the way things are done right now.

@shamil-gadelshin
Copy link
Contributor

This is a different PieceGetter with a different API and goal in mind. For example farmer's getter will reach out to archival storage and node's will only use L2 cache. So I don't see them as duplicates, they serve different purposes. At least the way things are done right now.

We don't have a limitation for nodes to reach L2 cache only when they need pieces to the best of my knowledge. Even if this is the case we can alter API to expose these options. This duplication (or similarity) is very close to our previous trait pair. Last time we put piece-getter into subspace-farmer-components to not introduce a new dependency. Future copy of some trait will justify creating a new crate.

@nazar-pc
Copy link
Member Author

nazar-pc commented Feb 5, 2024

We don't have a limitation for nodes to reach L2 cache only when they need pieces to the best of my knowledge

This is true, but use case is different. In case of farmer we care about individual specific pieces, hence retrieving them is a priority, even if we have to go to archival storage. In case of node we care about segment, so assuming at least 128/256 pieces are retrievable, we don't really care which ones and can afford to only pull pieces from L2 that is much quicker and better for the network overall. And if we don't get 128 only then we do retries rather than doing it on individual piece level.

Even if this is the case we can alter API to expose these options

Yes, but I'm not yet convinced having a single implementation with a growing number of optional features (like using or not using archival storage or doing retries) is a better experience. Maybe it is, but I initially looked into it when working on this PR and decided to not do that.

This duplication (or similarity) is very close to our previous trait pair. Last time we put piece-getter into subspace-farmer-components to not introduce a new dependency. Future copy of some trait will justify creating a new crate.

As explained above, I don't see this as a copy. Farmer components need to get pieces from somewhere for plotting and other purposes. It doesn't matter if it comes from DSN (in case of #2402 for example pieces will come from RPC for sub-farmers), so in my mind it makes sense to have a purpose-built trait for that, which can be implemented in various ways (for example farmer implements it differently than Pulsar). In DSN sync we have a different use case and API might be slightly different as well, simpler in this case.

That said, feel free to create a PR showing unified API, maybe it is a better solution overall, but even then I wouldn't consider it to belong to networking.

@nazar-pc nazar-pc added this pull request to the merge queue Feb 5, 2024
@shamil-gadelshin
Copy link
Contributor

I'm not yet convinced having a single implementation with a growing number of optional features (like using or not using archival storage or doing retries) is a better experience.

I'd add that implementations could be different but with a single trait and supporting primitives.

@nazar-pc
Copy link
Member Author

nazar-pc commented Feb 5, 2024

I'd add that implementations could be different but with a single trait and supporting primitives.

Yes, doing that would probably be the first step of unification. The only minor concern there would be a slight confusion where there is one trait, but in Pulsar we would actually expect different implementations passed into different components, even though technically both could have used the same one.

Merged via the queue into main with commit a2e0318 Feb 5, 2024
9 checks passed
@nazar-pc nazar-pc deleted the piece-getter-for-dsn-sync branch February 5, 2024 09:11
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.

3 participants