[TASK] Make yanic more modular #33

genofire · 2017-03-07T18:56:46Z

restructure of yanic to make it more modular.

On this way other timeserial-databases by other community are possible.

corny

Package state unglücklich benannt. Vielleicht runtime, oder anders nennen?

corny · 2017-03-11T20:14:29Z

cmd/yanic/main.go

+	if DEBUGDATABASE_BOOTSTRAP && config.Debug.Enable {
+		db = debugdatabase.New(config)
+		defer db.Close()
+


Leerzeile entfernen

corny · 2017-03-11T20:17:41Z

respond/collector.go

-	coll.db.AddCounterMap(database.MeasurementModel, stats.Models)
+	coll.db.AddGlobal(stats, time.Now())
+	coll.db.AddCounterMap(database.CounterMeasurementFirmware, stats.Firmwares)
+	coll.db.AddCounterMap(database.CounterMeasurementModel, stats.Models)


Gibt es nur Measurements bei InfluxDB? Dann sollten diese Konstanten hier verschwinden oder anders benannt werden.
→ Separation of Concerns

corny · 2017-03-11T20:20:56Z

debugdatabase/file.go

@@ -0,0 +1,49 @@
+package debugdatabase


Beschreibung für das Package fehlt. Was macht es und wofür ist es gut?

corny · 2017-03-11T20:29:06Z

state/config.go

+	}
+	Debug struct {
+		Enable bool
+		File   string


Was macht eine Debug File ? →Dokumentation!

corny · 2017-03-11T20:31:49Z

influxdb/node.go

-// ToInflux Returns tags and fields for InfluxDB
-func (node *Node) ToInflux() (tags imodels.Tags, fields imodels.Fields) {
+// NodeToInflux Returns tags and fields for InfluxDB
+func NodeToInflux(node *state.Node) (tags models.Tags, fields models.Fields) {


Wird von keinem anderen Package verwendet → nicht exportieren.

corny · 2017-03-11T20:35:19Z

meshviewer/graph.go

@@ -1,8 +1,10 @@
-package models
+package meshviewer


Sinnvoll 👍

corny · 2017-03-11T20:39:22Z

state/node.go

+type Flags struct {
+	Online  bool `json:"online"`
+	Gateway bool `json:"gateway"`
+}


Das flags struct ist offensichtlich für den MeshViewer? Dann gehört es dahin!
Das Node struct sollte dann Online und Gateway Felder bekommen, die anschließend für Mesh-Viewer kopiert werden.
→ Separation of Concerns

corny · 2017-03-11T20:52:24Z

cmd/yanic/build.debug.go

+const (
+	INFLUXDB_BOOTSTRAP      = true
+	DEBUGDATABASE_BOOTSTRAP = true
+)


Dokumentation fehlt 👎

corny · 2017-03-11T20:52:30Z

cmd/yanic/build.default.go

+const (
+	INFLUXDB_BOOTSTRAP      = true
+	DEBUGDATABASE_BOOTSTRAP = false
+)


Dokumentation fehlt 👎

genofire · 2017-03-16T12:27:20Z

runtime/config.go

 		DeleteInterval Duration // Delete stats of nodes every n minutes
 		DeleteAfter    Duration // Delete stats of nodes till now-deletetill n minutes
+		Connection     map[string]interface{}


@corny i do not like to use a map[string] but toml only allow struct or map[string] have you a other idea?
I want a field in Database which is abstract und could implemented diffrent by every database type.
Like in https://github.com/FreifunkBremen/yanic/pull/33/files#diff-9eb190bdda93006411f7ae456063a52aR16 (for exampledatabase) and https://github.com/FreifunkBremen/yanic/pull/33/files#diff-d2461a240b36089e7a2f4b124b4b96c9 (for influxdb)

Just put the database type in the key:

[database.influx] # ... [database.logging] # ...

corny · 2017-03-16T20:47:00Z

cmd/yanic/main.go

+	if config.Database.Enable {
+		db = connectDB(config)
+	}
+	defer database.Close(db)


db can be nil

corny · 2017-03-16T20:51:36Z

database/exampledatabase/file.go

@@ -0,0 +1,55 @@
+package exampledatabase


What do you think about calling it logging ?

corny · 2017-03-16T20:52:48Z

config_example.toml

+address  = "http://localhost:8086"
+database = "ffhb"
+username = ""
+password = ""


Which kind of database is database? How do you select the database kind?
Add a type or adapter.

corny · 2017-03-16T21:04:56Z

runtime/config.go

 		DeleteInterval Duration // Delete stats of nodes every n minutes
 		DeleteAfter    Duration // Delete stats of nodes till now-deletetill n minutes
+		Connection     map[string]interface{}


Just put the database type in the key:

[database.influx] # ... [database.logging] # ...

corny · 2017-03-16T21:06:04Z

meshviewer/nodes.go

+			Flags: Flags{
+				Online:  nodeOrigin.Online,
+				Gateway: nodeOrigin.Gateway,
+			},


corny · 2017-03-16T21:07:43Z

respond/collector.go

 )

 // Collector for a specificle respond messages
 type Collector struct {
 	connection *net.UDPConn   // UDP socket
 	queue      chan *Response // received responses
 	iface      string
-	db         *database.DB
-	nodes      *models.Nodes
+	db         database.DB


We should support multiple databases at the same time: db []database.DB

should we? lets talk tomorrow

i have no added a database called multi

corny · 2017-03-26T06:59:07Z

database/multi/main.go

+	}
+	if config.Database.Logging.Enable {
+		dbs = append(dbs, logging.New(config))
+	}


Die datenbank-Packages sollen sich selbst registrieren. Beispiel:
https://github.com/influxdata/telegraf/blob/v0.10.1/plugins/outputs/influxdb/influxdb.go#L158

corny · 2017-03-30T21:44:44Z

database/all/internal.go

+	return &DB{config: config, dbs: dbs}
+}
+func (this *DB) AddNode(nodeID string, node *runtime.Node) {
+	for _, db := range this.dbs {


Mein Linter sagt:

receiver name should be a reflection of its identity; don't use generic names such as "this" or "self"

corny · 2017-03-30T21:45:57Z

runtime/config.go

+type ConfigDatabaseLogging struct {
+	Enable bool
+	Path   string
+}


Separation of concerns ... das gehört in die Packages der Datenbank-Adapter.

corny · 2017-03-30T21:47:33Z

database/database.go

-	timer.Stop()
-	db.wg.Done()
+func AddConnection(n New) {
+	DBConnections = append(DBConnections, n)


Das sind nicht die DB-Connections, sondern Adapter, die eine DB-Connection erstellen können.
Die Adapter benutzen diese Funktion, um sich zu registrieren.

corny · 2017-03-30T21:48:07Z

database/database.go

-	}
-}
+// DB interface to use for implementation in e.g. influxdb
+type DB interface {


Besser: Adapter

corny · 2017-03-30T21:54:11Z

database/influxdb/database.go

+}
+
+func init() {
+	database.AddConnection(func(config *runtime.Config) database.DB {


Übergib kein runtime.Config - der Adapter interessiert sich nur für seine Verbindungsdaten.
Zurückgegeben wird eine eine Instanz des Adapters - also eine Session oder Verbindung (Connection) und keine Datenbank (DB).

corny

Travis-Build schlägt fehlt.

genofire · 2017-04-01T10:42:12Z

Ja da er noch nicht richtig dei Config einliest:
überleg ein toml fork auf zu machen, toml:"-" funktioniert nicht richtig und hidden entries im struct werden dennoch versucht zu befüllen.
Für die angefangene implementation bräuchte ich viele hidden Funktion aus den toml package.

Hast du eine besser idee, wollte mich noch ein bisschen in reflection einlesen ... find die aktuelle angefange implentation für die Config schrecklich

genofire · 2017-04-01T10:42:31Z

Darfst gern weitermachen, komme vor montag nicht dazu

corny · 2017-04-02T15:15:45Z

database/logging/file.go

+	database.AddConnect("logging", Connect)
+}
+
+func Connect(configuration interface{}) database.Connection {


Don't ignore errors - return them.

corny · 2017-04-02T15:23:10Z

database/database.go

-		// write batch now?
-		if bp != nil && (writeNow || closed || len(bp.Points()) >= batchMaxSize) {
-			log.Println("saving", len(bp.Points()), "points")
+var Connects = map[string]Connect{}


Connects or Connections ?
I think you mean adapters.

Is DatabaseTypes also okay?

Rename it to Adapters. Does it have to be exported?

Its needed here: https://github.com/FreifunkBremen/yanic/blob/cleanup-package/database/all/internal.go#L19 - We need a export ...

genofire · 2017-04-02T18:22:02Z

Add both labels, because .... - This PR is in main develop, and had to switch so often the labels

genofire · 2017-04-04T04:58:39Z

Soll ich schon mal vor squashen?

corny · 2017-04-04T07:46:58Z

database/influxdb/database.go

+	if !config.Enable() {
+		return nil, errors.New("connection disabled")
+	}
+	log.Println("init database: influxdb")


move this line to the caller function

corny · 2017-04-04T07:47:43Z

database/logging/file.go

+	if !config.Enable() {
+		return nil, errors.New("connection disabled")
+	}
+	log.Println("init database: logging")


move this line to the caller function

corny · 2017-04-04T07:47:56Z

database/logging/file.go

+	log.Println("init database: logging")
+	file, err := os.OpenFile(config.Path(), os.O_APPEND|os.O_WRONLY|os.O_CREATE, 0600)
+	if err != nil {
+		log.Println("File could not opened: ", config.Path())


print errors in the caller function

corny · 2017-04-04T07:50:49Z

database/all/internal.go

+	list []database.Connection
+}
+
+func Connect(configuration interface{}) database.Connection {


This method should do something helpful if an unknown adapter is configured.

?? Stehe auf den schlauch ... was meinst du damit? Connect soll mit database.Connect equivalent bleiben.

Was passiert, wenn man einen unbekannten Adapter und die Konfiguration schreibt und welches Verhalten wünscht man sich als sysadmin?

corny · 2017-04-04T12:54:16Z

database/all/internal.go

-	return &Connection{list: list}
+	if len(list) <= 0 {
+		return nil, errors.New("it was not possible to create any database connection")
+	}


Ich finde Yanik soll auch ohne Datenbank funktionieren.

Jep tut es auch, wenn man es ausschaltet -> f0a6195#diff-690f51ef4035e92812f351daa99158fbR45

egal, wurde dort ausgenommen

genofire · 2017-04-07T19:47:21Z

@corny wie schaut es aus?

corny · 2017-04-10T15:30:55Z

runtime/config.go

+		Enable         bool     `toml:"enable"`
+		DeleteInterval Duration `toml:"delete_interval"` // Delete stats of nodes every n minutes
+		DeleteAfter    Duration `toml:"delete_after"`    // Delete stats of nodes till now-deletetill n minutes
+		Connection     map[string][]interface{}


Das Enable soll zu einer Datenbankverbindung gehören.

in Zeile 35, nein global.
Bei uns/influxdb gibt es noch einmal ein enable (unter Connection)

Keine Gute Idee?

genofire · 2017-04-10T16:06:13Z

config_example.toml

 # cleaning data of measurement node,
 #   which are older than 7d
 delete_after = "7d"
 #   how often run the cleaning
 delete_interval = "1h"
+
+[[database.connection.influxdb]]
+enable   = false


Einmal ob generell in datenbanken gespeichert werden soll in Zeile 58
und hier wiederum, ob diese Datenbank benutzt werden soll.

corny · 2017-04-10T16:23:26Z

cmd/yanic/main.go

-		}
-		database.Start(db, config)
-		defer database.Close(db)
+	db, err = all.Connect(config.Database.Connection)


Sind das mehrere Datenbanken? connections wäre ein besserer Namen für eine Liste von Verbindungen.

genofire force-pushed the cleanup-package branch 6 times, most recently from d136516 to 627262b Compare March 7, 2017 20:52

genofire requested a review from corny March 7, 2017 20:58

corny requested changes Mar 11, 2017

View reviewed changes

genofire added enhancement wip labels Mar 12, 2017

genofire force-pushed the cleanup-package branch 2 times, most recently from a0d5ba9 to 27d5f43 Compare March 13, 2017 17:39

genofire added review and removed wip labels Mar 14, 2017

genofire force-pushed the cleanup-package branch 2 times, most recently from 73afb72 to aed177f Compare March 15, 2017 22:37

genofire commented Mar 16, 2017

View reviewed changes

genofire changed the title ~~Make yanic more modular~~ [TASK] Make yanic more modular Mar 16, 2017

corny requested changes Mar 16, 2017

View reviewed changes

genofire force-pushed the cleanup-package branch from 29bb856 to dbf6a00 Compare March 16, 2017 23:07

genofire added wip and removed review labels Mar 18, 2017

genofire force-pushed the cleanup-package branch 3 times, most recently from 702baf3 to c22281e Compare March 19, 2017 10:05

genofire added review and removed wip labels Mar 19, 2017

genofire force-pushed the cleanup-package branch from c22281e to 12ea186 Compare March 21, 2017 15:03

corny requested changes Mar 26, 2017

View reviewed changes

genofire added review and removed wip labels Mar 29, 2017

corny requested changes Mar 30, 2017

View reviewed changes

genofire force-pushed the cleanup-package branch from a0cc4da to 57bc520 Compare March 31, 2017 21:11

corny requested changes Apr 1, 2017

View reviewed changes

genofire mentioned this pull request Apr 2, 2017

Add support for hopglass nodes.json and graph.json #50

Closed

corny requested changes Apr 2, 2017

View reviewed changes

genofire added the wip label Apr 2, 2017

genofire force-pushed the cleanup-package branch from 82bd068 to 823cae2 Compare April 2, 2017 18:35

corny requested changes Apr 4, 2017

View reviewed changes

genofire mentioned this pull request Apr 6, 2017

Rename respond-collector to yanic FreifunkBremen/ansible#77

Closed

genofire force-pushed the cleanup-package branch from 04fc994 to efa2d33 Compare April 9, 2017 08:05

genofire added a commit that referenced this pull request Apr 9, 2017

[TASK] Make yanic more modular for multiple databases (#33)

efa2d33

genofire removed the wip label Apr 9, 2017

corny requested changes Apr 10, 2017

View reviewed changes

genofire commented Apr 10, 2017

View reviewed changes

corny requested changes Apr 10, 2017

View reviewed changes

genofire force-pushed the cleanup-package branch 2 times, most recently from b4570a3 to fed58a4 Compare April 10, 2017 16:39

corny approved these changes Apr 10, 2017

View reviewed changes

[TASK] Make yanic more modular for multiple databases (#33)

7da3b13

genofire force-pushed the cleanup-package branch from fed58a4 to 7da3b13 Compare April 10, 2017 16:50

genofire merged commit f135249 into master Apr 10, 2017

genofire deleted the cleanup-package branch April 10, 2017 16:54

[TASK] Make yanic more modular #33

[TASK] Make yanic more modular #33

Conversation

genofire commented Mar 7, 2017

corny 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

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

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

Choose a reason for hiding this comment

genofire Mar 18, 2017 • edited Loading

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

Choose a reason for hiding this comment

corny left a comment

Choose a reason for hiding this comment

genofire commented Apr 1, 2017

genofire commented Apr 1, 2017

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

genofire commented Apr 2, 2017

genofire commented Apr 4, 2017

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

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

genofire commented Apr 7, 2017

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

genofire Mar 18, 2017 •

edited

Loading