-
Notifications
You must be signed in to change notification settings - Fork 292
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
[config] Add p2p.disc.concurrency flag #3829
Conversation
@@ -54,6 +54,8 @@ const ( | |||
DefaultWSPort = 9800 | |||
// DefaultPrometheusPort is the default prometheus port. The actual port used is 9000+900 | |||
DefaultPrometheusPort = 9900 | |||
// DefaultP2PConcurrency is the default P2P concurrency, 0 means is set the default value of P2P Discovery, the actual value is 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the default value from libp2p?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment mentioned, 10 is default
please fix travis test. |
cmd/harmony/config_migrations.go
Outdated
@@ -103,6 +103,10 @@ func init() { | |||
confTree.Set("P2P.IP", defaultConfig.P2P.IP) | |||
} | |||
|
|||
if confTree.Get("P2P.Concurrency") == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a new field, and you need to update the config version. Also it is recommended that you rebase on top of #3773
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to wait the PR you mentioed merged, and I can rebase on the top of the PR. Otherwise this PR may still be changed
cmd/harmony/flags.go
Outdated
@@ -513,6 +513,11 @@ var ( | |||
DefValue: defaultConfig.P2P.KeyFile, | |||
Deprecated: "use --p2p.keyfile", | |||
} | |||
p2pDiscoveryConcurrencyFlag = cli.IntFlag{ | |||
Name: "p2p.concurrency", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest p2p.disc.concurrency
. p2p.concurrency is too general
p2p/discovery/discovery.go
Outdated
@@ -42,6 +42,13 @@ func NewDHTDiscovery(host libp2p_host.Host, opt DHTConfig) (Discovery, error) { | |||
if err != nil { | |||
return nil, err | |||
} | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest move to getLibp2pRawOptions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't do two things in one PR. You may either downgrade or change the default value in the flag. we have no evidence that the libp2p upgrade will cause the issue.
b08947d
to
1bf1a53
Compare
Removed downgrade kad-dht code in |
1bf1a53
to
2f8dd26
Compare
This reverts commit af7e0de.
Issue
Because of the block time problem of all shards, the parameter of concurrency needs to be exposed for adjustment. If it is set to 1, it may currently go wrong and affect the consensus. Set to 1, according to the paper, KAD DHT query time will downgrade to Chord‘s algorithm
NOTE: Add p2p.disc.concurrency flag and default value with libp2p dht
Test
Passed Local test