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

[stream] added stream manager module #3554

Merged
merged 4 commits into from
Mar 1, 2021
Merged

Conversation

JackyWYX
Copy link
Contributor

Description

This is a breakdown PR of #3535 and is on top of #3552.

  1. Added stream manager module to manage over streams of a stream protocol.

Stream manager do the following job:

  1. add a new stream.
  2. remove a stream.
  3. discover and connect new streams when the number of streams is below threshold.
  4. emit stream events to inform other modules.
  5. reset all streams on close.

@JackyWYX JackyWYX self-assigned this Feb 26, 2021
@JackyWYX JackyWYX mentioned this pull request Feb 26, 2021
Copy link
Contributor

@LeoHChen LeoHChen left a comment

Choose a reason for hiding this comment

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

LGTM in general.

const (
// checkInterval is the default interval for checking stream number. If the stream
// number is smaller than softLoCap, an active discover through DHT will be triggered.
checkInterval = 30 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

how are these numbers chosen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By estimation without solid evidence. Basically this is just like estimating the timeout to do a HTTP request. The only trick here is to make discTimeout less than checkInterval to avoid simultaneous host/client stream setup. Do you have other references that these numbers can be analyzed upon?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can probably observe the timeout by adding prometheus metrics and change to a more solid value after launch. For now I think the number is good to start with. Do you other references that can be helped to determine these number's values?

@rlan35 rlan35 merged commit 8adce89 into harmony-one:main Mar 1, 2021
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.

3 participants