Collapse PeerConfig into P2PConfig by xla · Pull Request #1676 · tendermint/tendermint

xla · 2018-06-02T15:32:33Z

As both configs are concerned with the p2p packaage and PeerConfig is only used inside of the package there is no good reason to keep the couple of fields separate, therefore it is collapsed into the more general P2PConifg. This is a stepping stone towards a setup where the components inside of p2p do not have any knowledge about the config.

follow-up to #1325

codecov-io · 2018-06-03T12:42:22Z

Codecov Report

Merging #1676 into develop will decrease coverage by 0.03%.
The diff coverage is 70.4%.

@@             Coverage Diff             @@
##           develop    #1676      +/-   ##
===========================================
- Coverage    62.13%   62.09%   -0.04%     
===========================================
  Files          121      121              
  Lines        11094    11120      +26     
===========================================
+ Hits          6893     6905      +12     
- Misses        3583     3597      +14     
  Partials       618      618

Impacted Files	Coverage Δ
p2p/fuzz.go	`0% <0%> (-9.86%)`	⬇️
config/config.go	`82.79% <100%> (+1.29%)`	⬆️
p2p/test_util.go	`56.47% <100%> (ø)`	⬆️
p2p/conn/connection.go	`81.76% <100%> (ø)`	⬆️
p2p/peer.go	`60.81% <63.63%> (-1.28%)`	⬇️
p2p/switch.go	`52.96% <88.23%> (+0.7%)`	⬆️
consensus/reactor.go	`77.24% <0%> (-1.08%)`	⬇️
blockchain/pool.go	`68.88% <0%> (-0.7%)`	⬇️
mempool/mempool.go	`71.35% <0%> (+0.2%)`	⬆️
p2p/pex/pex_reactor.go	`72.96% <0%> (+0.65%)`	⬆️
... and 1 more

melekes · 2018-06-04T07:05:47Z

p2p/test_util.go

 	"github.com/tendermint/tmlibs/log"

-	cfg "github.com/tendermint/tendermint/config"
+	"github.com/tendermint/tendermint/config"


third change of this sort on my memory. we should make a standard and stick with it

I made it consistent inside of p2p in the follow-up PR which comes right after this one is through.

melekes · 2018-06-04T07:07:39Z

p2p/switch.go


-func (sw *Switch) addInboundPeerWithConfig(conn net.Conn, config *PeerConfig) error {
+func (sw *Switch) addInboundPeerWithConfig(
+	conn net.Conn,


Is this the result of default formatter? or you manually splitting the line

This is how arguments are split when ranged over multiple lines.

melekes · 2018-06-04T07:08:15Z

p2p/switch.go

-	sw.peerConfig.MConfig.SendRate = config.SendRate
-	sw.peerConfig.MConfig.RecvRate = config.RecvRate
-	sw.peerConfig.MConfig.MaxPacketMsgPayloadSize = config.MaxPacketMsgPayloadSize
+	sw.config.MConfig.FlushThrottle = time.Duration(cfg.FlushThrottleTimeout) * time.Millisecond


I thought the whole idea was to get rid of this block after merge.

Coming next, I first wanted to collapse configs and in the next step get rid of the config dependency completey.

It will another 2-3 distinct PRs, I want to avoid massive changesets for this.

melekes · 2018-06-04T07:09:47Z

p2p/conn/connection.go

 // DefaultMConnConfig returns the default config.
-func DefaultMConnConfig() *MConnConfig {
-	return &MConnConfig{
+func DefaultMConnConfig() MConnConfig {


all our Default..Config functions return pointers. Maybe we should be consistent

Using pointers turned out to be racy, in general I would challenge that those should be mutable. Having them non-pointers would communicate that better. On other other hand we can always dereference them where they are consumed.

melekes · 2018-06-04T07:11:49Z

config/config.go

+	// Peer connection configuration.
+	HandshakeTimeout time.Duration      `mapstructure:"handshake_timeout"`
+	DialTimeout      time.Duration      `mapstructure:"dial_timeout"`
+	MConfig          tmconn.MConnConfig `mapstructure:"connection"`


should this be a pointer too?

Same point as above, it shouldn't be mutated.

As both configs are concerned with the p2p packaage and PeerConfig is only used inside of the package there is no good reason to keep the couple of fields separate, therefore it is collapsed into the more general P2PConifg. This is a stepping stone towards a setup where the components inside of p2p do not have any knowledge about the config. follow-up to #1325

ebuchman · 2018-06-05T01:45:59Z

p2p/peer.go

+	p.Logger.Debug(
+		"Unknown channel for peer",
+		"channel",
+		chID,


we should keep the pairs on a single line

Will include in the next one.

ebuchman · 2018-06-05T01:47:57Z

p2p/pex/pex_reactor_test.go


 func createSwitchAndAddReactors(reactors ...p2p.Reactor) *p2p.Switch {
-	sw := p2p.MakeSwitch(config, 0, "127.0.0.1", "123.123.123", func(i int, sw *p2p.Switch) *p2p.Switch { return sw })
+	sw := p2p.MakeSwitch(cfg, 0, "127.0.0.1", "123.123.123", func(i int, sw *p2p.Switch) *p2p.Switch { return sw })


maybe it would be better to make it more explicit by calling it testCfg or something ?

That it is global state is already a smell, it's gonna vanish when all the config traces are gone from the package.

…#1676) * Revert "ADR 109: Modularize test infra (tendermint#1488)" This reverts commit e4a82ef64128786e1e2ec3a5d60a3be3a48918d9. Signed-off-by: Thane Thomson <connect@thanethomson.com> * Remove part of ADR 109 that deals with modularization of E2E test infra Signed-off-by: Thane Thomson <connect@thanethomson.com> * Mark ADR 109 as accepted Signed-off-by: Thane Thomson <connect@thanethomson.com> * Add tracking issue to ADR 109 Signed-off-by: Thane Thomson <connect@thanethomson.com> --------- Signed-off-by: Thane Thomson <connect@thanethomson.com>

xla added the C:p2p Component: P2P pkg label Jun 2, 2018

xla self-assigned this Jun 2, 2018

xla requested review from ebuchman and melekes June 2, 2018 15:32

xla force-pushed the xla/collapse-peerconfig branch 2 times, most recently from f8bae31 to 9ec56fd Compare June 3, 2018 12:26

xla force-pushed the xla/collapse-peerconfig branch from 9ec56fd to c749e3f Compare June 3, 2018 15:19

melekes reviewed Jun 4, 2018

View reviewed changes

xla force-pushed the xla/collapse-peerconfig branch 2 times, most recently from 63b2e98 to ea89686 Compare June 5, 2018 00:06

ebuchman approved these changes Jun 5, 2018

View reviewed changes

ebuchman merged commit fd4db8d into develop Jun 5, 2018

ebuchman deleted the xla/collapse-peerconfig branch June 5, 2018 01:50

Conversation

xla commented Jun 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Jun 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xla commented Jun 2, 2018 •

edited

Loading

codecov-io commented Jun 3, 2018 •

edited

Loading