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

[RFC] influxdb: replace log.Fatal() by log.Print() #61

Merged
merged 1 commit into from
Jun 29, 2017

Conversation

kb-light
Copy link
Contributor

Fatal() calls os.Exit(1), which causes yanic.service to fail if database is not reachable.

@coveralls
Copy link

coveralls commented Apr 21, 2017

Coverage Status

Coverage remained the same at 56.272% when pulling 8ae3dbc on kb-light:log into d20277a on FreifunkBremen:master.

@kb-light kb-light changed the title influxdb: replace log.Fatal() by log.Print() [RFC] influxdb: replace log.Fatal() by log.Print() Apr 21, 2017
@corny
Copy link
Member

corny commented Apr 27, 2017

I didn't test it yet: Will the client send data, as soon as the database becomes reachable again?

@genofire
Copy link
Member

genofire commented May 28, 2017

On a Fatal/panic kills yanic, so you got a nice log to find the problem.
Systemd could restart yanic and put it to sleep, if it crashed to often.
(if the database is not accessibly over a longer time).

A Print will keep yanic running.
If the client has a internal reconnect this PR is okay.
But if not, we have to run the Connect again .... (so this PR is no solution).

@kb-light
Copy link
Contributor Author

The goal is to keep yanic running, even if the database is not available, to have at least the latest data available for the map.
I am having no problems with this PR: If the database is not reachable for some minutes, yanic keeps running and if the database is reachable again, the new data is stored. But I have not tested it with log downtimes of the database.

@@ -114,7 +114,7 @@ func (conn *Connection) addWorker() {
// create new batch
timer.Reset(batchTimeout)
if bp, err = client.NewBatchPoints(bpConfig); err != nil {
log.Fatal(err)
log.Print(err)
Copy link
Member

Choose a reason for hiding this comment

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

NewBatchPoints only fails if the configured precision can not be passed. In that case it will always fail and should panic:
https://github.com/influxdata/influxdb/blob/3b700863eaa3fae65a995ce65ca12fc85c0a6995/client/v2/client.go#L213

@@ -134,7 +134,7 @@ func (conn *Connection) addWorker() {
log.Println("saving", len(bp.Points()), "points")

if err = conn.client.Write(bp); err != nil {
log.Fatal(err)
log.Print(err)
Copy link
Member

Choose a reason for hiding this comment

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

That should be fine since the InfluxDB client uses net/http that has timeouts and reconnects.

Copy link
Member

Choose a reason for hiding this comment

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

Cool

@genofire genofire added wip and removed question labels Jun 1, 2017
Fatal() calls os.Exit(1), which causes yanic.service to fail if database is not reachable.
@coveralls
Copy link

coveralls commented Jun 23, 2017

Coverage Status

Coverage remained the same at 57.688% when pulling d39de38 on kb-light:log into 550e76a on FreifunkBremen:master.

@@ -149,7 +149,7 @@ func (conn *Connection) addWorker() {
log.Println("saving", len(bp.Points()), "points")

if err = conn.client.Write(bp); err != nil {
log.Fatal(err)
log.Print(err)
}
writeNow = false
bp = nil
Copy link
Member

Choose a reason for hiding this comment

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

@corny: Müsste dieses bp = nil nicht in ein else Zweig? Damit die daten nicht verloren gehen?

Copy link
Member

Choose a reason for hiding this comment

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

Kann man machen, dann stellt sich aber die Frage, wie lange man Punkte sammeln will.

Copy link
Contributor Author

@kb-light kb-light Jun 25, 2017

Choose a reason for hiding this comment

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

Mir geht es erst einmal darum, dass yanic für die Auslieferung der aktuellen Daten weiterläuft und sich wieder mit der influxdb verbindet, wenn diese wieder online ist. Dass die Daten aus der Offline-Zeit nicht in der DB landen, entspricht ja auch der Erwartung.

Generell wäre es natürlich schick, wenn man auch diese Daten sammeln kann und nachträglich in der DB anlegt, aber da sehe ich auch das Problem der Zwischenspeicherung.

@kb-light
Copy link
Contributor Author

@genofire @corny wie bekommen wir denn diesen PR mal gemerged? Passt das jetzt erst einmal so, oder werden noch Änderungen gewünscht?

@corny corny merged commit fd7e712 into FreifunkBremen:master Jun 29, 2017
@corny
Copy link
Member

corny commented Jun 29, 2017

Passt! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants