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

Implement SearchValue/GetValue #942

Merged
merged 26 commits into from
Sep 28, 2023
Merged

Conversation

dennis-tra
Copy link
Contributor

@dennis-tra dennis-tra commented Sep 27, 2023

fixes #911
fixes #912

Todo

  • flaky test
  • fixing peers that store "worse" records than the one we have found.
  • all // TODO

@dennis-tra dennis-tra added the v2 All issues related to the v2 rewrite label Sep 27, 2023
@dennis-tra dennis-tra marked this pull request as ready for review September 27, 2023 13:52
v2/config.go Outdated Show resolved Hide resolved
@dennis-tra
Copy link
Contributor Author

Related: #945

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.

Looks good, just a few comments, nothing too substantial

v2/backend.go Outdated Show resolved Hide resolved
v2/internal/coord/coordinator.go Show resolved Hide resolved
v2/routing.go Outdated Show resolved Hide resolved
// acknowledgement that the other peer has received the data is not
// guaranteed. The data will be flushed at this point, but the remote might
// not have handled it yet. Therefore, we use "EventuallyWithT" here.
assert.EventuallyWithT(t, func(t *assert.CollectT) {
Copy link

Choose a reason for hiding this comment

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

We control both peers in this test so can't we deterministically query the remote peer to see if the data has been handled?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the function body, I'm querying the remote peer to see if the data has been handled. Because of the reasons in the comment above I'm not sure how we can do this deterministically.

I could pass a custom Backend/Datastore to d2 and use a wait group to block until it's stored on the other peer 🤔

v2/routing_test.go Outdated Show resolved Hide resolved
v2/routing_test.go Show resolved Hide resolved

func TestDHT_Bootstrap_no_peers_configured(t *testing.T) {
// TIMING: this test is based on timeouts - so might become flaky!
ctx := kadtest.CtxShort(t)
Copy link

Choose a reason for hiding this comment

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

We could run timing based tests for longer. CtxShort is for tests that we expect to complete quickly - either the result is processed quickly or it never happens because something is broken. We could have another helper function that always returns a context with a deadline just before the test deadline so the test has the full allotted time to complete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both of the timing-sensitive tests run very fast. It's just that the state doesn't manifests at the remote peers immediately.

@dennis-tra dennis-tra merged commit 03adce6 into v2-develop Sep 28, 2023
11 checks passed
@dennis-tra dennis-tra deleted the v2-issue-911-search-value branch September 28, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2 All issues related to the v2 rewrite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants