Fix Default node mishandling batch groupBy tags #1233
Conversation
|
@nathanielc This could probably use a bit more testing to guarantee that the tags are properly set on both the points in a batch and the batch itself. If you think adding those tests would be worthwhile, I'm happy to add them. |
|
Not sure why this is yellow. It's been green for awhile now. |
CHANGELOG.md
Outdated
| @@ -30,6 +30,9 @@ | |||
| Specifically in the JSON result data the old key `Series` is always `series`, and the old key `Err` is now always `error` instead of for only some of the outputs. | |||
| - [#1181](https://github.com/influxdata/kapacitor/pull/1181): Fix bug parsing dbrp values with quotes. | |||
| - [#1228](https://github.com/influxdata/kapacitor/pull/1228): Fix panic on loading replay files without a file extension. | |||
| - [#1192](https://github.com/influxdata/kapacitor/issues/1192): BREAKING: Fix bug in Default Node not updating batch tags and groupID. | |||
| @@ -84,7 +87,7 @@ func (d *DefaultNode) setDefaults(fields models.Fields, tags models.Tags) (model | |||
| newTags := tags | |||
| tagsCopied := false | |||
| for tag, value := range d.d.Tags { | |||
| if _, ok := tags[tag]; !ok { | |||
| if v := tags[tag]; v == "" { | |||
There was a problem hiding this comment.
Ah, I see this is the breaking change. I guess we should break things :) Should we enable this behavior behind a flag? Something like
|default()
.tag('dc', 'sfc')
.defaultEmptyTags()
There was a problem hiding this comment.
While it is breaking, I think that this is the correct functionality. The reason being that it's not possible to write a point in line protocol with an empty tag value and therefore and points that would have meet this condition would be points where someone grouped by a tag that did not exist. If I'm a user, I would have to know when I group by something that doesn't exist, that the resulting data will be tagged with the non existent tag, but with an empty string and so I much use the defaultEmptyTags().
The counter argument to this is that it is a breaking change, and we shouldn't break things that people may be relying on.
I'm happy to go whichever route you think is best.
There was a problem hiding this comment.
The reason being that it's not possible to write a point in line protocol with an empty tag value
I didn't realize this. Since it is true, (I just tested it) it is not possible that someone was relying on the subtle difference in behavior.
So lets make the change as is but not mark it as BREAKING since it is not actually possible that someone was relying on the difference.
Add Changelog entry for #1233 Remove BREAKING from changelog entry
Add Changelog entry for influxdata#1233 Remove BREAKING from changelog entry
Required for all non-trivial PRs