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

Decouple announce Senders from Publishers #88

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

gammazero
Copy link
Collaborator

@gammazero gammazero commented Jul 21, 2023

There is no strong reason to associate a set of announce Senders with a Publisher. The only association is that a Sender is often used after updating the Sublisher's head CID. Separating these allows more flexibility in choosing where serving an advertisement chain is done and where sending advertisement announcements is done.

Fixes #87

There is no strong reason to associate a set of announce Senders with a Publisher. The only association is that a Sender is often used to after updating the Sublisher's head CID. By separating these, it allows more flexibility in where in handling serving an advertisement chain is done and where sending advertisement announcements is done.
@gammazero gammazero requested a review from masih July 21, 2023 23:55
@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2023

Codecov Report

Patch coverage: 70.27% and project coverage change: -0.17 ⚠️

Comparison is base (9116d75) 53.64% compared to head (c8bd3b7) 53.48%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #88      +/-   ##
==========================================
- Coverage   53.64%   53.48%   -0.17%     
==========================================
  Files          65       65              
  Lines        5244     5155      -89     
==========================================
- Hits         2813     2757      -56     
+ Misses       2131     2106      -25     
+ Partials      300      292       -8     
Impacted Files Coverage Δ
announce/httpsender/option.go 54.83% <0.00%> (-13.17%) ⬇️
dagsync/dtsync/option.go 57.14% <ø> (-15.59%) ⬇️
announce/httpsender/sender.go 63.97% <14.28%> (-2.70%) ⬇️
announce/sender.go 60.00% <60.00%> (ø)
dagsync/p2p/protocol/head/head.go 74.22% <81.81%> (+0.26%) ⬆️
announce/p2psender/option.go 83.33% <100.00%> (+8.33%) ⬆️
announce/p2psender/sender.go 51.16% <100.00%> (+5.00%) ⬆️
dagsync/dtsync/publisher.go 48.43% <100.00%> (-2.03%) ⬇️
dagsync/httpsync/publisher.go 66.66% <100.00%> (+7.10%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@MichaelMure
Copy link

This is so much better, thank you :-)

@gammazero gammazero merged commit 2f35326 into main Jul 24, 2023
16 checks passed
@gammazero gammazero deleted the separate-publishers-senders branch July 24, 2023 13:55
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.

Publisher and Sender interface overlap
4 participants