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.

Sign up for GitHub

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

Jump to bottom

Add broadcast state machine for storing records in the DHT #930

Merged
merged 30 commits into from
Sep 22, 2023
Merged

Add broadcast state machine for storing records in the DHT #930

merged 30 commits into from
Sep 22, 2023

Conversation

dennis-tra
Copy link
Contributor

fixes: #914

@dennis-tra dennis-tra added the v2 All issues related to the v2 rewrite label Sep 19, 2023
@dennis-tra dennis-tra self-assigned this Sep 19, 2023
@dennis-tra dennis-tra force-pushed the v2-issue-914-put-value branch 3 times, most recently from 824afd7 to d4da2c8 Compare September 20, 2023 12:26
@dennis-tra dennis-tra changed the base branch from v2-develop to v2-routing-getvalue2 September 20, 2023 13:33
@dennis-tra dennis-tra force-pushed the v2-issue-914-put-value branch 2 times, most recently from 2e5e248 to 20c19a7 Compare September 21, 2023 11:34
@dennis-tra dennis-tra marked this pull request as ready for review September 21, 2023 11:38
iand
iand reviewed Sep 21, 2023
View reviewed changes
v2/internal/coord/brdcst/brdcst_followup.go Outdated Show resolved Hide resolved
v2/internal/coord/brdcst/brdcst_followup.go Outdated Show resolved Hide resolved
v2/internal/coord/brdcst/brdcst_followup.go Outdated Show resolved Hide resolved
v2/internal/coord/brdcst/brdcst_followup.go Outdated
return &StateBroadcastFinished[K, N]{
QueryID: st.QueryID,
Contacted: make([]N, 0),
Errors: map[string]struct {
Copy link

Choose a reason for hiding this comment

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

Maybe add a timeout error here

v2/internal/coord/brdcst/brdcst_followup.go Outdated Show resolved Hide resolved
v2/internal/coord/brdcst/pool.go Outdated Show resolved Hide resolved
v2/internal/coord/brdcst/pool.go Outdated Show resolved Hide resolved
v2/internal/coord/brdcst/pool_states.go Outdated Show resolved Hide resolved
v2/internal/coord/brdcst/pool_states.go Outdated Show resolved Hide resolved
v2/internal/coord/brdcst_events.go Show resolved Hide resolved
Base automatically changed from v2-routing-getvalue2 to v2-develop September 21, 2023 14:13
@dennis-tra dennis-tra changed the title feat: implement PutValue in v2 (wip) Add broadcast state machine for storing records in the DHT Sep 22, 2023
iand
iand reviewed Sep 22, 2023
View reviewed changes
v2/internal/coord/brdcst.go
"github.com/libp2p/go-libp2p-kad-dht/v2/tele"
)

type PooledBroadcastBehaviour struct {
Copy link

Choose a reason for hiding this comment

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

Add comment for this type

iand
iand reviewed Sep 22, 2023
View reviewed changes
v2/internal/coord/coordt/coretypes.go
@@ -11,6 +11,15 @@ import (
"github.com/libp2p/go-libp2p-kad-dht/v2/pb"
)

// TODO: rename to something like OperationID. This type isn't only used to identify queries but also other operations like broadcasts.
Copy link

Choose a reason for hiding this comment

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

Agree. OperationID seems a good name for it.

iand
iand approved these changes Sep 22, 2023
View reviewed changes
Copy link

@iand iand left a comment

Choose a reason for hiding this comment

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

LGTM

A few comments that can be done here or in a follow up

@dennis-tra dennis-tra merged commit 74ffa67 into v2-develop Sep 22, 2023
11 checks passed
@dennis-tra dennis-tra deleted the v2-issue-914-put-value branch September 22, 2023 11:27
@dennis-tra
Copy link
Contributor Author

Will do in a follow-up 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iand iand iand approved these changes

Assignees

@dennis-tra dennis-tra

Labels
v2 All issues related to the v2 rewrite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dennis-tra @iand