issue_64_fix #258
issue_64_fix #258
Conversation
issue_64_fix
change github root from graphite-ng to scrichar
|
Have tested locally, this fixes #64 |
destination/destination.go
Outdated
| @@ -49,6 +50,7 @@ type Destination struct { | |||
| SpoolSyncPeriod time.Duration | |||
| SpoolSleep time.Duration // how long to wait between stores to spool | |||
| UnspoolSleep time.Duration // how long to wait between loads from spool | |||
| RouteName string // how long to wait between loads from spool | |||
There was a problem hiding this comment.
this comment doesn't make any sense
There was a problem hiding this comment.
yeah, my bad, copied from line above, will remove that
|
this PR looks pretty good. have to analyze a bit deeper though. |
|
You do not need to specify a spool name, the spoolName would now be derived from a combination of the routeName and the destination/port. Thus providing a way to have unique spool names across different routes with the same destination/port. When the carbon-relay starts up after an upgrade with this fix, the relay would create brand new spools, and essentially any spool that existed before the upgrade would be orphaned. Perhaps I could add a note in the ReadME, for example: "When upgrading to a release > v0.9.4, verify any critical data in the spools is emptied, or the data is not critical and can be orphaned as spool naming convention has changed." |
|
sounds good. don't worry about the readme. we can put that in the release notes once we make a release |
util/util.go
Outdated
| func SpoolKeyAddrToPath(s string, routeName string) string { | ||
| s = AddrToPath(s) | ||
| sList := []string{routeName, s} | ||
| return strings.Join(sList, "_") |
There was a problem hiding this comment.
this is unnecessarily complicated
can just return routeName + "_" + AddrToPath(s)
|
note also that we currently set so my suggestion is , rename the spoolKey property to simply 'Key', and use it for all stats and log calls. this way we solve the problem for spool files as well as log messages as well as stats. |
adding fixes for comments on logs and stats to use new dest key inste…
|
@Dieterbe , I have tested these changes locally, things are looking good on my fork. Logs and stats are now using the new property "key". cleanAddr is removed. Spooling is now using the combination of the route name and the destination/port combo. Can you review? |
|
looks pretty good. thanks a lot for your contribution, this was a longstanding pain point and i'm glad you stepped up :) |
Fixes #64 for our use