-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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';
a5ecc36
to
8b0ea13
Compare
m.seriesIDs = ids | ||
sort.Sort(m.seriesIDs) | ||
|
||
// add this series id to the tag index on the measurement |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: comment looks wrong.
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. |
a01cb5f
to
d35e2a9
Compare
} | ||
} | ||
m.seriesIDs = ids | ||
sort.Sort(m.seriesIDs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And actually, an even better way to go would be to pass this thing a collection of structs:
type seriesDrop struct {
measurmentName string
ids []uint316
Then 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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