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

Khalil/1899 async rpc sent tracker #4553

Merged
merged 36 commits into from
Jul 25, 2023
Merged

Conversation

kc1116
Copy link
Contributor

@kc1116 kc1116 commented Jul 12, 2023

This PR adds a worker pool to the gossipsub mesh tracer rpc sent tracker worker pool. RPC control messages are processed synchronously, so it's imperative that any sub-process involved in the processing of RPC's must be non-blocking. This PR also updates the tracker to keep track of the last highest iHaves size, this will be used during iWant flooding detection to dynamically update the sample size of iWants expected based on the most recent largest iHaves sent.

Further reading: https://github.com/dapperlabs/flow-go/issues/6472

@kc1116 kc1116 requested review from thep2p and peterargue as code owners July 12, 2023 15:49
@kc1116 kc1116 requested a review from gomisha July 12, 2023 15:49
@codecov-commenter
Copy link

codecov-commenter commented Jul 12, 2023

Codecov Report

Merging #4553 (2f95232) into master (b53dd43) will increase coverage by 0.03%.
The diff coverage is 75.21%.

@@            Coverage Diff             @@
##           master    #4553      +/-   ##
==========================================
+ Coverage   54.44%   54.48%   +0.03%     
==========================================
  Files         912      912              
  Lines       85165    85253      +88     
==========================================
+ Hits        46372    46453      +81     
- Misses      35216    35225       +9     
+ Partials     3577     3575       -2     
Flag Coverage Δ
unittests 54.48% <75.21%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
module/component/component.go 89.92% <ø> (ø)
module/metrics/herocache.go 0.00% <0.00%> (ø)
network/p2p/p2pbuilder/libp2pNodeBuilder.go 0.00% <0.00%> (ø)
network/netconf/flags.go 25.80% <50.00%> (-0.57%) ⬇️
network/p2p/tracer/internal/rpc_sent_tracker.go 87.67% <86.76%> (-12.33%) ⬇️
network/p2p/tracer/gossipSubMeshTracer.go 94.32% <91.30%> (+0.06%) ⬆️
network/p2p/tracer/internal/cache.go 100.00% <100.00%> (ø)

... and 13 files with indirect coverage changes

@kc1116 kc1116 requested a review from thep2p July 18, 2023 23:06
Copy link
Contributor

@thep2p thep2p left a comment

Choose a reason for hiding this comment

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

Looks great, except the two comments I made, the rest is good 🚀 .

if err != nil {
return fmt.Errorf("failed to get track rpc work nonce: %w", err)
}
if ok := t.workerPool.Submit(trackableRPC{Nonce: n, rpc: rpc}); !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add tests for the following scenarios:

  1. Duplicate RPCs with different nonce values; both should be processed.
  2. Distinct RPCs tracked concurrently; a worker should eventually pick each.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kc1116 kc1116 requested a review from peterargue July 20, 2023 23:30
// All worker functions will be run in parallel when the ComponentManager is started.
// Note: AddWorker is not concurrency-safe, and should only be called on an individual builder
// within a single goroutine.
// within a single goroutine.// AddWorker adds a ComponentWorker closure to the ComponentManagerBuilder
Copy link
Contributor

Choose a reason for hiding this comment

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

the formatting here is messed up. Can you revert the changes to this file?

@kc1116
Copy link
Contributor Author

kc1116 commented Jul 25, 2023

bors merge

bors bot added a commit that referenced this pull request Jul 25, 2023
4553: Khalil/1899 async rpc sent tracker r=kc1116 a=kc1116

This PR adds a worker pool to the gossipsub mesh tracer rpc sent tracker worker pool. RPC control messages are processed synchronously, so it's imperative that any sub-process involved in the processing of RPC's must be non-blocking. This PR also updates the tracker to keep track of the `last highest iHaves size`, this will be used during iWant flooding detection to dynamically update the sample size of iWants expected based on the most recent largest iHaves sent.

Further reading: https://github.com/dapperlabs/flow-go/issues/6472

Co-authored-by: Khalil Claybon <[email protected]>
@bors
Copy link
Contributor

bors bot commented Jul 25, 2023

@kc1116 kc1116 merged commit 3a8139b into master Jul 25, 2023
@kc1116 kc1116 deleted the khalil/1899-async-rpc-sent-tracker branch July 25, 2023 21:10
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.

4 participants