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

bitswap(client/messageque): expose donthavetimeout config #703

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Wondertan
Copy link
Member

Closes #701

@Wondertan Wondertan requested a review from a team as a code owner October 24, 2024 03:13
@Wondertan Wondertan changed the title expose donthavetimeout config bitswap(client/messageque): expose donthavetimeout config Oct 24, 2024
Copy link

codecov bot commented Oct 24, 2024

Codecov Report

Attention: Patch coverage is 86.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 60.33%. Comparing base (392493d) to head (d04bd72).
Report is 63 commits behind head on main.

Files with missing lines Patch % Lines
bitswap/client/client.go 20.00% 4 Missing ⚠️
...client/internal/messagequeue/donthavetimeoutmgr.go 94.44% 1 Missing and 1 partial ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #703      +/-   ##
==========================================
- Coverage   60.35%   60.33%   -0.03%     
==========================================
  Files         243      243              
  Lines       31038    31051      +13     
==========================================
  Hits        18734    18734              
- Misses      10637    10648      +11     
- Partials     1667     1669       +2     
Files with missing lines Coverage Δ
...tswap/client/internal/messagequeue/messagequeue.go 83.53% <100.00%> (-1.39%) ⬇️
...client/internal/messagequeue/donthavetimeoutmgr.go 95.39% <94.44%> (+2.12%) ⬆️
bitswap/client/client.go 86.74% <20.00%> (-1.34%) ⬇️

... and 8 files with indirect coverage changes

@Wondertan Wondertan force-pushed the donthavetimeoutconfig branch from c9ca8a0 to d04bd72 Compare October 24, 2024 03:38
activeWants: make(map[cid.Cid]*pendingWant),
timeout: cfg.DontHaveTimeout,
messageLatency: &latencyEwma{alpha: cfg.MessageLatencyAlpha},
onDontHaveTimeout: onDontHaveTimeout,
Copy link
Contributor

Choose a reason for hiding this comment

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

The onDontHaveTimeout function seems to accompany the config is many/most places. So, onDontHaveTimeout callback may be a candidate for moving into the DontHaveTimeoutConfig type.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess the question is: "Do we want users to register a custom handler there?". I would be opposed to that unless there is a prominent use case.

Comment on lines +13 to +23
func defaultDontHaveTimeoutConfig() *DontHaveTimeoutConfig {
cfg := &DontHaveTimeoutConfig{
DontHaveTimeout: 5 * time.Second,
MaxExpectedWantProcessTime: 2 * time.Second,
PingLatencyMultiplier: 3,
MessageLatencyAlpha: 0.5,
MessageLatencyMultiplier: 2,
}

cfg.MaxTimeout = cfg.DontHaveTimeout + cfg.MaxExpectedWantProcessTime
return cfg
Copy link
Member

Choose a reason for hiding this comment

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

Two concerns with this approach:

  1. We expose a way for overriding the defaults without exposing the implicit default values themselves
    • people will keep hardcoding current values instead of referencing const that may change in the future
  2. We use a struct which forces override fo all values
    • This makes it tedious to override a single value
    • This means if we add a new knob, people who already override the struct will override default with implicit zero value for that type (likely causing bugs)

My mental model for makign things like this configurable is to

  • have WithFoo configuration options which allow overriding each knob individually
  • and expose DefaultFoo to allow people to programmatically refer to implicit default, rather than hardcoding them in their own code.

This may be more work in this PR upfront, but will save us from headache in the future. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

I suggest we expose the default struct for people to override necessary knobs this way:

cfg := DefaultDontHaveTimeoutConfig()
// example of granular customization
cfg.MaxExpectedWantProcessTime = 5 * time.Second
// use the global option.
WithDontHaveTimeoutConfig(cfg)

This solves your concern without creating "options hell". This is equivalent to having multi-level options, yet it's cleaner. Also, it doesn't require forwarding all the options from client to bitswap in case you want to add more options horizontally, which I tried initially, and got lost.

Copy link
Member Author

@Wondertan Wondertan Oct 29, 2024

Choose a reason for hiding this comment

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

Basically, the compromise I propose it expose the default configuration solving your concern while keeping things straightforward.

@gammazero gammazero added need/author-input Needs input from the original author status/blocked Unable to be worked further until needs are met labels Oct 29, 2024
@Wondertan
Copy link
Member Author

@gammazero, @lidel, one thing I realized about this component is that it's actually never disabled. The option to disable donthavetimeout handling exists in bitswap. However, it simply makes the handler no-op, while the component is still active and running, doing work that is costly (measuring latency, firing timeouts in routines, etc). And disabled donthavetimeout is the default behaviour in Kubo.

@lidel lidel removed the status/blocked Unable to be worked further until needs are met label Nov 12, 2024
@lidel
Copy link
Member

lidel commented Nov 12, 2024

Triage notes:

  • exposing defaults sgtm
    • nit: make it public? defaultDontHaveTimeoutConfig → DefaultDontHaveTimeoutConfig`
  • @Wondertan what do you mean by "And disabled donthavetimeout is the default behaviour in Kubo."? Mind linking to relevant code/config?

@lidel lidel added need/author-input Needs input from the original author and removed need/author-input Needs input from the original author labels Nov 12, 2024
@gammazero gammazero assigned gammazero and unassigned lidel Dec 10, 2024
@gammazero gammazero added need/maintainers-input Needs input from the current maintainer(s) and removed need/author-input Needs input from the original author labels Dec 10, 2024
@gammazero
Copy link
Contributor

I am planning on changing this to all the onDontHaveTime config and enable/disable using an option param. This will likely be done in a different PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/maintainers-input Needs input from the current maintainer(s)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bitswap(messagequeue): Expose DONT_HAVE timeout knobs
3 participants