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

blob_v1 and blob_piece_v1 dependency replication #514

Merged
merged 18 commits into from
Sep 22, 2023

Conversation

sandreae
Copy link
Member

@sandreae sandreae commented Aug 18, 2023

blob_v1 and blob_piece_v1 documents should only be replicated if they are related to from another document which is included because of schema specified in the target set. For example:

  • TargetSet([img_0020, blob_v1]) would included all img_0020 documents and any blob_v1 documents they relate to
  • TargetSet([img_0020, blob_v1, blob_piece_v1]) would included all img_0020 documents and any blob_v1 documents they relate to, and any blob_piece_v1 documents they relate to
  • TargetSet([img_0020]) would included all img_0020 documents and nothing else
  • TargetSet([blob_v1, blob_piece_v1]) would not include anything

Next step is to add a config flag which adds the correct blob system schema to the target set for you: #564

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@sandreae sandreae changed the base branch from main to garbage-collection August 18, 2023 18:58
@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

Patch coverage: 99.23% and project coverage change: +0.06% 🎉

Comparison is base (c968e0d) 92.34% compared to head (1e3b5ee) 92.41%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
+ Coverage   92.34%   92.41%   +0.06%     
==========================================
  Files         106      106              
  Lines       17861    18062     +201     
==========================================
+ Hits        16494    16692     +198     
- Misses       1367     1370       +3     
Files Changed Coverage Δ
aquadoggo/src/replication/ingest.rs 89.47% <ø> (ø)
aquadoggo/src/replication/strategies/log_height.rs 97.34% <98.84%> (+0.57%) ⬆️
aquadoggo/src/db/stores/blob.rs 98.18% <100.00%> (+0.19%) ⬆️
aquadoggo/src/db/stores/entry.rs 99.11% <100.00%> (-0.04%) ⬇️
aquadoggo/src/replication/manager.rs 87.91% <100.00%> (+0.21%) ⬆️
aquadoggo/src/replication/session.rs 89.28% <100.00%> (+0.32%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sandreae sandreae changed the base branch from garbage-collection to development August 31, 2023 18:03
@sandreae sandreae linked an issue Aug 31, 2023 that may be closed by this pull request
@sandreae sandreae changed the base branch from development to main September 8, 2023 19:43
@sandreae sandreae marked this pull request as ready for review September 19, 2023 15:30
Copy link
Member

@adzialocha adzialocha 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! I find the new "rules" quite intuitive. Obviously that will change the "magic demo" character of aquadoggo slightly, as you can't just replicate blobs anymore like that. You'll need a schema first (aka fishy to create one) which has a relation to a blob!

@sandreae
Copy link
Member Author

Thank you! I find the new "rules" quite intuitive. Obviously that will change the "magic demo" character of aquadoggo slightly, as you can't just replicate blobs anymore like that. You'll need a schema first (aka fishy to create one) which has a relation to a blob!

Thanks 🙏 glad it's intuitive.

Yeh, I'd considered how this effects our "it just works" node setup. I thought about adding a rule that if you only have blob_v1 and blob_piece_v1 then you magically replicate blobs again. Wasn't sure if that actually just made things more complicated though 🤔 once we have multiple sync sessions between peers this can be nice addition. Probably with config changes we can have a nice "it just works" setup again.

@sandreae
Copy link
Member Author

We could easily have an i_want_all_the_blobs flag which over-rides this behaviour.

@adzialocha
Copy link
Member

We could easily have an i_want_all_the_blobs flag which over-rides this behaviour.

Yeah, let's merge this for now, I think we need some more time to think about a good UI for configuration.

@sandreae
Copy link
Member Author

We could easily have an i_want_all_the_blobs flag which over-rides this behaviour.

Yeah, let's merge this for now, I think we need some more time to think about a good UI for configuration.

💯 stack it ✊

@sandreae sandreae merged commit 27c5069 into main Sep 22, 2023
8 checks passed
@sandreae sandreae deleted the blob-dependency-replication branch September 22, 2023 09:18
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.

Dependency replication for blobs
2 participants