Wire up drop series parsing #1629
Conversation
|
@dgnorton Take a look at this to see if I'm on the right track. |
| if s.Source != nil { | ||
| _, _ = buf.WriteString("FROM ") | ||
| _, _ = buf.WriteString(s.Source.String()) | ||
| if s.Condition != nil { |
There was a problem hiding this comment.
The if s.Condition block should be at the same level as the if s.Source because you can have either of them with or without the other. e.g.,...
DROP SERIES FROM cpu;
DROP SERIES WHERE region = 'uswest';
DROP SERIES FROM cpu WHERE region = 'uswest';
corylanou
force-pushed
the
wire-up-drop-series
branch
5 times, most recently
from
a5ecc36
to
8b0ea13
Compare
| m.seriesIDs = ids | ||
| sort.Sort(m.seriesIDs) | ||
|
|
||
| // add this series id to the tag index on the measurement |
|
Some comments. Also, please add a test that does the following:
Need some confidence that the in memory tag index is still in good shape. |
corylanou
force-pushed
the
wire-up-drop-series
branch
from
a01cb5f
to
d35e2a9
Compare
| } | ||
| } | ||
| m.seriesIDs = ids | ||
| sort.Sort(m.seriesIDs) |
There was a problem hiding this comment.
I'm pretty sure this sort call is unnecessary since they're already in sorted order and appends should preserve that.
|
|
||
| c := measurmentBucket.Cursor() | ||
|
|
||
| for k, _ := c.First(); k != nil; k, _ = c.Next() { |
There was a problem hiding this comment.
Why are you looping through with a cursor? You have the seriesID, you can just delete that key
There was a problem hiding this comment.
Also, drop series should take a collection of seriesIDs. That way you can delete many all in a single bolt transaction
There was a problem hiding this comment.
Drop series takes measurement names, and the series and measurements may not line up right?
DROP SERIES where host='server01'would go across different measurement names.
There was a problem hiding this comment.
This dropSeries method takes a single measurement name. So you should only call it with series that exist in the given measurement you're passing in. Even so, doing a delete on a non-existent key is better than running a cursor over everything.
There was a problem hiding this comment.
And actually, an even better way to go would be to pass this thing a collection of structs:
type seriesDrop struct {
measurmentName string
ids []uint316Then you pass that array to the drop series method and you can remove everything in a single tx.
| } | ||
|
|
||
| // Delete series from the database. | ||
| if err := database.dropSeries(c.SeriesIDs...); err != nil { |
There was a problem hiding this comment.
This dropSeries should also be updated to take the seriesByMeasurement map. That way you're not looping through every single measurement and trying to drop it
|
Ok, made those two last comments about passing around measurement names. Then I noticed this: https://github.com/influxdb/influxdb/pull/1629/files#diff-34c6b408d72845d076d47126c29948d1R2102 You should be attaching the series ids to the measurement names from the very start. You have them there anyway. The And that's what you should be passing around everywhere. Would make everything cleaner and faster. |
| // Remove series information from measurements | ||
| m := db.measurements[measurement] | ||
| if !m.dropSeries(id) { | ||
| return fmt.Errorf("failed to remove series id %d from measurment %q", id, m.Name) |
There was a problem hiding this comment.
This shouldn't throw an error. If it was already removed, we're ok with that, just continue on the loop
| func (s *Shard) deleteSeries(name string) error { | ||
| panic("not yet implemented") // TODO | ||
| func (s *Shard) dropSeries(seriesID uint32) error { | ||
| return s.store.Update(func(tx *bolt.Tx) error { |
There was a problem hiding this comment.
You should check first that s.store != nil. If this shard isn't local to the server, it will be, causing an error here.
|
LGTM, |
!
Fixes #1422