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

Reply to "wants" messages with "has" messages #12

Merged
merged 7 commits into from
Jul 1, 2022
Merged

Conversation

boreq
Copy link
Contributor

@boreq boreq commented Jun 29, 2022

This implements replying to "wants" messages with "has" messages. "Wants" messages are received through blobs.createWants stream initiated by the local node and have to trigger a reply with a "has" message through blobs.createWants stream initiated by the remote node.

@boreq boreq changed the title Reply to wants Reply to "wants" messages with "has" messages Jun 30, 2022
// Remote: pub.Identity(),
// Address: network2.NewAddress("one.planetary.pub:8008"),
//}
//myPatchwork = refs.MustNewIdentity("@qFtLJ6P5Eh9vKxnj7Rsh8SkE6B6Z36DVLP7ZOKNeQ/Y=.ed25519")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not important, this whole file is just a test program not used in production.

@@ -0,0 +1,29 @@
package replication
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the code in this file was moved here from other files.

downloader Downloader
logger logging.Logger

// todo cleanup processes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this can be done later, it could even stay like this all the way to production unless we ran the app for a very long time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

As I understand the underlying process goroutines for incoming and outgoing loops will get terminated on connection context termination anyway, right? So the only thing we'd have to improve would be reacting to connection termination by deleting the process struct from this map (for example by running a goroutine awaiting ctx close while creating a new process in getOrCreateProcess). Do I understand the problem correctly?
I'm actually ok with leaving it as it is if that's only about cleaning up the process struct. As I can imagine the map won't grow huge in our normal use case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

@boreq boreq marked this pull request as ready for review June 30, 2022 14:43
Comment on lines +75 to +81
wl, err := p.wantListStorage.GetWantList()
if err != nil {
p.logger.WithError(err).Error("could not get the want list")
continue
}

for _, v := range wl.List() {
Copy link
Collaborator

@czeslavo czeslavo Jul 1, 2022

Choose a reason for hiding this comment

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

I think that we are missing dropping a blob from the wants list once we fetch it. That means we will always tell all our peers about all of our wants what is not optimal I guess. Should we add this to the todo list in #1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this to #1. The want list has to be redone in general, it is completely temporary.

@boreq boreq merged commit 5a6dbb7 into feature/blobs Jul 1, 2022
@boreq boreq deleted the reply-to-wants branch July 1, 2022 15:03
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.

2 participants