moved ConnectionWatcherType from p2p.toml to prefs.toml #4299

raduchis · 2022-07-15T15:45:20Z

the ConnectionWatcherType was overwritten on upgrade and so was moved to prefs.toml
added emptyConnectionsWatcher const so that if the ConnectionWatcherType is not set, there should be no need to add it to the prefs.toml

codecov-commenter · 2022-07-15T15:54:57Z

Codecov Report

❗ No coverage uploaded for pull request base (rc/2022-july@dcb5d50). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff               @@
##             rc/2022-july    #4299   +/-   ##
===============================================
  Coverage                ?   73.67%           
===============================================
  Files                   ?      662           
  Lines                   ?    86132           
  Branches                ?        0           
===============================================
  Hits                    ?    63458           
  Misses                  ?    17917           
  Partials                ?     4757

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcb5d50...da95fe1. Read the comment docs.

…prefs.toml

bogdan-rosianu · 2022-07-18T07:25:52Z

cmd/seednode/main.go

+		PreferredPeersHolder:  disabled.NewPreferredPeersHolder(),
+		NodeOperationMode:     p2p.NormalOperation,
+		PeersRatingHandler:    disabled.NewDisabledPeersRatingHandler(),
+		ConnectionWatcherType: "disabled",


that flag hasn't been altered in the previous code. also, might use constants

seednode will not work with ConnectionWatcherType

one more config for the seednode :(
Since it won't work on the seednode binary anyway, I suggest to leave it as a const.

bogdan-rosianu · 2022-07-18T07:27:04Z

config/prefsConfig.go

@@ -13,4 +13,5 @@ type PreferencesConfig struct {
 	RedundancyLevel            int64
 	PreferredConnections       []string
 	FullArchive                bool
+	ConnectionWatcherType      string


please leave FullArchive bool the last one due to memory alignment

moved up one position

bogdan-rosianu · 2022-07-18T07:27:31Z

integrationTests/factory/componentsHelper.go

@@ -52,6 +52,7 @@ func CreateDefaultConfig() *config.Configs {

 	p2pConfig.KadDhtPeerDiscovery.Enabled = false
 	prefsConfig.Preferences.DestinationShardAsObserver = "0"
+	prefsConfig.Preferences.ConnectionWatcherType = "print"


please define constants somewhere for these values

added them to p2p (ConnectionWatcherType interface is declared here) constants and used them everywhere

bogdan-rosianu · 2022-07-18T07:28:22Z

p2p/libp2p/metrics/factory/connectionWatcherFactory.go

-const printConnectionsWatcher = "print"
-const disabledConnectionsWatcher = "disabled"
+const (
+	printConnectionsWatcher    = "print"


move them in a new package and export them & use everywhere

why exported?

added them to p2p and used everywhere

bogdan-rosianu · 2022-07-18T07:29:19Z

p2p/libp2p/metrics/factory/connectionWatcherFactory.go


 // NewConnectionsWatcher creates a new ConnectionWatcher instance based on the input parameters
 func NewConnectionsWatcher(connectionsWatcherType string, timeToLive time.Duration) (p2p.ConnectionsWatcher, error) {
 	switch connectionsWatcherType {
 	case printConnectionsWatcher:
 		return metrics.NewPrintConnectionsWatcher(timeToLive)
-	case disabledConnectionsWatcher:
+	case disabledConnectionsWatcher, emptyConnectionsWatcher:


might use a new case for emptyConnectionsWatcher because it can only be empty due to bad configuration. therefore, we'd want to see a log warn

I agree to be left as it is now: the config misconfiguration will be hit because the prefs.toml is not overwritten on upgrade.

yes, the idea to add the emptyConnectionsWatcher without any error is that the prefs.toml is not overwritten and has to be added manually. Update log.Trace to log.Debug on the connectionsWatcherTypeFactory

iulianpascalau

minor stuff 👍

iulianpascalau · 2022-07-18T07:55:27Z

p2p/libp2p/metrics/factory/connectionWatcherFactory.go

-const printConnectionsWatcher = "print"
-const disabledConnectionsWatcher = "disabled"
+const (
+	printConnectionsWatcher    = "print"


why exported?

iulianpascalau · 2022-07-18T07:57:53Z

p2p/libp2p/metrics/factory/connectionWatcherFactory.go


 // NewConnectionsWatcher creates a new ConnectionWatcher instance based on the input parameters
 func NewConnectionsWatcher(connectionsWatcherType string, timeToLive time.Duration) (p2p.ConnectionsWatcher, error) {
 	switch connectionsWatcherType {
 	case printConnectionsWatcher:
 		return metrics.NewPrintConnectionsWatcher(timeToLive)
-	case disabledConnectionsWatcher:
+	case disabledConnectionsWatcher, emptyConnectionsWatcher:


I agree to be left as it is now: the config misconfiguration will be hit because the prefs.toml is not overwritten on upgrade.

iulianpascalau · 2022-07-18T07:58:36Z

p2p/libp2p/netMessenger.go

@@ -182,7 +183,8 @@ func constructNode(
 		return nil, err
 	}

-	connWatcher, err := metricsFactory.NewConnectionsWatcher(args.P2pConfig.Node.ConnectionWatcherType, ttlConnectionsWatcher)
+	log.Trace("connectionWatcherType", "type", args.ConnectionWatcherType)


can be log.Debug as it is a one-time print

changed to Debug

iulianpascalau · 2022-07-18T07:59:15Z

p2p/libp2p/netMessenger_test.go

+		SyncTimer:             &libp2p.LocalSyncTimer{},
+		PreferredPeersHolder:  &p2pmocks.PeersHolderStub{},
+		PeersRatingHandler:    &p2pmocks.PeersRatingHandlerStub{},
+		ConnectionWatcherType: "print",


can be an exported const in export_test.go that will be equal to the unexported const

added to p2p as constant (where ConnectionWatcherType interface is declared) and used everywhere

…ype-prefs.toml' into EN-12618-move-connectionWatcherType-prefs.toml

…prefs.toml

popenta

@@ Log scanner @@

EN-12618-move-connectionWatcherType-prefs.toml##

================================================================================

Known Warnings 22
New Warnings 0
Known Errors 0
New Errors 2
Panics 0
================================================================================

…prefs.toml

moved ConnectionWatcherType from p2p.toml to prefs.toml

ae0ba2a

Merge branch 'rc/2022-july' into EN-12618-move-connectionWatcherType-…

209cb60

…prefs.toml

bogdan-rosianu self-requested a review July 18, 2022 07:02

bogdan-rosianu requested changes Jul 18, 2022

View reviewed changes

iulianpascalau self-requested a review July 18, 2022 07:48

iulianpascalau reviewed Jul 18, 2022

View reviewed changes

raduchis added 4 commits July 18, 2022 12:38

fixed after review

84d23be

Merge remote-tracking branch 'origin/EN-12618-move-connectionWatcherT…

ac5ead3

…ype-prefs.toml' into EN-12618-move-connectionWatcherType-prefs.toml

fixed unit tests and cyclic imports after merge

c620027

Merge branch 'rc/2022-july' into EN-12618-move-connectionWatcherType-…

34dbff7

…prefs.toml

iulianpascalau approved these changes Jul 19, 2022

View reviewed changes

bogdan-rosianu approved these changes Jul 19, 2022

View reviewed changes

Merge branch 'rc/2022-july' into EN-12618-move-connectionWatcherType-…

da95fe1

…prefs.toml

popenta approved these changes Jul 21, 2022

View reviewed changes

Merge branch 'rc/2022-july' into EN-12618-move-connectionWatcherType-…

68cbd12

…prefs.toml

raduchis self-assigned this Jul 21, 2022

Merge branch 'rc/2022-july' into EN-12618-move-connectionWatcherType-…

d5c3ffb

…prefs.toml

iulianpascalau merged commit 0b0d466 into rc/2022-july Jul 22, 2022

iulianpascalau deleted the EN-12618-move-connectionWatcherType-prefs.toml branch July 22, 2022 18:02

iulianpascalau added complexity:trivial config impact:low interested:protocol interested:validator size:small type:refactoring priority:4-!i&!u Not Important and not Urgent labels Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

moved ConnectionWatcherType from p2p.toml to prefs.toml #4299

moved ConnectionWatcherType from p2p.toml to prefs.toml #4299

raduchis commented Jul 15, 2022

codecov-commenter commented Jul 15, 2022 •

edited

bogdan-rosianu Jul 18, 2022

raduchis Jul 18, 2022 •

edited

iulianpascalau Jul 18, 2022

bogdan-rosianu Jul 18, 2022

raduchis Jul 18, 2022

bogdan-rosianu Jul 18, 2022

raduchis Jul 18, 2022 •

edited

bogdan-rosianu Jul 18, 2022

iulianpascalau Jul 18, 2022

raduchis Jul 19, 2022

bogdan-rosianu Jul 18, 2022

iulianpascalau Jul 18, 2022

raduchis Jul 18, 2022 •

edited

iulianpascalau left a comment

iulianpascalau Jul 18, 2022

iulianpascalau Jul 18, 2022

iulianpascalau Jul 18, 2022

raduchis Jul 18, 2022

iulianpascalau Jul 18, 2022

raduchis Jul 18, 2022 •

edited

popenta left a comment

moved ConnectionWatcherType from p2p.toml to prefs.toml #4299

moved ConnectionWatcherType from p2p.toml to prefs.toml #4299

Conversation

raduchis commented Jul 15, 2022

codecov-commenter commented Jul 15, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

raduchis Jul 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raduchis Jul 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raduchis Jul 18, 2022 • edited

Choose a reason for hiding this comment

iulianpascalau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raduchis Jul 18, 2022 • edited

Choose a reason for hiding this comment

popenta left a comment

Choose a reason for hiding this comment

EN-12618-move-connectionWatcherType-prefs.toml##

codecov-commenter commented Jul 15, 2022 •

edited

raduchis Jul 18, 2022 •

edited

raduchis Jul 18, 2022 •

edited

raduchis Jul 18, 2022 •

edited

raduchis Jul 18, 2022 •

edited