fix #1717: all points must have at least one field #1718
Conversation
| // Make sure every point has at least one field. | ||
| for _, p := range points { | ||
| if len(p.Fields) == 0 { | ||
| return 0, fmt.Errorf("point %s has no fields", p.Name) |
There was a problem hiding this comment.
Just return ErrFieldsRequired. In any event, cpu is a Measurement.
There was a problem hiding this comment.
To be clear, ErrFieldsRequired already exists in the source.
There was a problem hiding this comment.
Also, why not do this at line 1403? We are already walking the points there. Let's not walk at the top, then walk again.
There was a problem hiding this comment.
At line 1403, the Measurements and ShardGroups have been created...they would have to be deleted. Or, we'd have to move creation of measurements and shard groups inside the loop and created them one at a time. Maybe that would give better performance?...not sure.
There was a problem hiding this comment.
OK, I see what you mean. Ironically createMeasurementsIfNotExists also loops through the points and I wrote that.
Now that I see it, this function does a lot of stuff with a batch -- create shards, create series etc. It now seems to me you're on the right path, and that some basic validation of the batch is quite important before anything else happens. It kills me that we loop through the batch so many times. Oh well, we can profile it and see how it is, if it comes to it.
|
Update the changelog and address @otoolep's comment. Other than that LGTM. |
|
Change makes sense, but it seems to me that the code should use the existing point-walking loop. |
|
+1 on this change now, it's pretty important that the batch is sane before anything else happens. |
|
+1, as long as the error code is fixed up, of course. |
fix #1717: all points must have at least one field
No description provided.