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

provider: refactor to only maintain one batched implementation and add throughput callback #273

Merged
merged 2 commits into from
Jun 1, 2023

Conversation

Jorropo
Copy link
Contributor

@Jorropo Jorropo commented Apr 6, 2023

This code does a few things:

  1. Remove the simple provider to avoid duplicating features.
  2. Add the support for single providing on the batched provider.
  3. Fix a bugs in the batched provider.
  4. Add support for a throughputCallback in the batched provider.
  5. Add support for an offline mode of the batched provider (stuff is exclusively pushed onto the queue).
  6. Move the batched provider to be the only provider and make the queue implementation private.

@Jorropo Jorropo requested a review from a team as a code owner April 6, 2023 13:24
@Jorropo Jorropo force-pushed the provider branch 4 times, most recently from f450d31 to e5be496 Compare April 6, 2023 14:58
@hacdias
Copy link
Member

hacdias commented May 8, 2023

@Jorropo can you update this once you have time so that the tests are passing? I can take a look after (also add some motivation on why this is beneficial for more context, please).

@Jorropo Jorropo force-pushed the provider branch 2 times, most recently from 1a119c1 to 8d9e801 Compare May 10, 2023 09:40
@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Merging #273 (07350aa) into main (c23df38) will increase coverage by 1.08%.
The diff coverage is 50.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #273      +/-   ##
==========================================
+ Coverage   49.36%   50.44%   +1.08%     
==========================================
  Files         281      279       -2     
  Lines       33708    33586     -122     
==========================================
+ Hits        16640    16944     +304     
+ Misses      15329    14886     -443     
- Partials     1739     1756      +17     
Impacted Files Coverage Δ
provider/noop.go 0.00% <0.00%> (ø)
provider/provider.go 0.00% <0.00%> (ø)
provider/reprovider.go 70.41% <76.19%> (ø)
provider/internal/queue/queue.go 76.76% <100.00%> (ø)

... and 10 files with indirect coverage changes

@Jorropo Jorropo force-pushed the provider branch 11 times, most recently from fb36b39 to cedadbf Compare May 11, 2023 17:50
@BigLep
Copy link
Contributor

BigLep commented May 11, 2023

Yes, agreed that the context for where this is coming from and why we're prioritizing it now would be great. It's not currently on the 0.21 iteration. (I'll add it now.)

@Jorropo Jorropo force-pushed the provider branch 2 times, most recently from 30d1ef9 to 80d8715 Compare May 11, 2023 19:16
@hacdias
Copy link
Member

hacdias commented May 12, 2023

Is this ready to be reviewed @Jorropo?

@BigLep
Copy link
Contributor

BigLep commented May 16, 2023

Related to ipfs/kubo#9703

@Jorropo Jorropo force-pushed the provider branch 3 times, most recently from a19c0c0 to 0827951 Compare May 17, 2023 09:03
Jorropo added a commit to Jorropo/go-ipfs that referenced this pull request May 19, 2023
Jorropo added a commit to Jorropo/go-ipfs that referenced this pull request May 22, 2023
@BigLep
Copy link
Contributor

BigLep commented May 23, 2023

2023-05-23 maintainer conversation:

Context: we know this will get rewritten again when it moves into the DHT. As a result, this doesn't have to be perfect.

@guillaumemichel : can you please look at this (given this is relevant to future code for the DHT)? We want to make sure you have context here when future DHT rewriting happens.

@aschmahmann will look at the test code.

@Jorropo : add a changelog.md entry

@guillaumemichel
Copy link
Contributor

The function below is not deterministic and only has a 50% chance to return an error when the context is done.

// Enqueue puts a cid in the queue
func (q *Queue) Enqueue(cid cid.Cid) error {
select {
case q.enqueue <- cid:
return nil
case <-q.ctx.Done():
return fmt.Errorf("failed to enqueue CID: shutting down")
}
}

I rewrote it, feel free to update if required

// Enqueue puts a cid in the queue
func (q *Queue) Enqueue(cid cid.Cid) error {
select {
case <-q.ctx.Done():
return fmt.Errorf("failed to enqueue CID: shutting down")
default:
}
select {
case q.enqueue <- cid:
return nil
case <-q.ctx.Done():
return fmt.Errorf("failed to enqueue CID: shutting down")
}
}

provider/internal/queue/queue.go Show resolved Hide resolved
provider/noop.go Outdated Show resolved Hide resolved
provider/provider.go Outdated Show resolved Hide resolved
provider/reprovider.go Outdated Show resolved Hide resolved
@guillaumemichel
Copy link
Contributor

I relocated some magic numbers in reprovider.go

const (
// MAGIC: how long we wait before reproviding a key
DefaultReproviderInterval = time.Hour * 22 // https://github.com/ipfs/kubo/pull/9326
// MAGIC: If the reprovide ticker is larger than a minute (likely), provide
// once after we've been up a minute. Don't provide _immediately_ as we
// might be just about to stop.
defaultInitialReprovideDelay = time.Minute
// MAGIC: how long we wait between the first provider we hear about and
// batching up the provides to send out
pauseDetectionThreshold = time.Millisecond * 500
// MAGIC: how long we are willing to collect providers for the batch after
// we receive the first one
maxCollectionDuration = time.Minute * 10
)

Note that I updated the DefaultReprovideInterval from 24h to 22h (see ipfs/kubo#9326 and https://github.com/ipfs/kubo/blob/da28fbc65a2e0f1ce59f9923823326ae2bc4f713/config/reprovider.go#L5)

Jorropo added a commit to Jorropo/go-ipfs that referenced this pull request May 31, 2023
Jorropo and others added 2 commits May 31, 2023 17:01
…d throughput callback

This code does a few things:
1. Remove the simple provider to avoid duplicating features.
2. Add the support for single providing on the batched provider.
3. Fix a bugs in the batched provider.
4. Add support for a throughputCallback in the batched provider.
6. Add support for an offline mode of the batched provider (stuff is exclusively pushed onto the queue).
5. Move the batched provider to be the only provider and make the queue implementation private.
@BigLep
Copy link
Contributor

BigLep commented May 31, 2023

@Jorropo : this is good to merge right?

@Jorropo
Copy link
Contributor Author

Jorropo commented May 31, 2023

@BigLep I'll submit changelog update first

Jorropo added a commit that referenced this pull request Jun 1, 2023
@Jorropo Jorropo merged commit 0962ed6 into main Jun 1, 2023
Jorropo added a commit that referenced this pull request Jun 1, 2023
@Jorropo Jorropo deleted the provider branch June 1, 2023 11:22
Jorropo added a commit to Jorropo/go-ipfs that referenced this pull request Jun 1, 2023
Jorropo added a commit to Jorropo/go-ipfs that referenced this pull request Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants