-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix: improve delete speed when a measurement is part of the predicate #23786
Conversation
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.
Some questions, which may lead to code changes, and some straight up code changes.
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.
One import formatting change.
tsdb/store.go
Outdated
@@ -6,6 +6,7 @@ import ( | |||
"context" | |||
"errors" | |||
"fmt" | |||
errors3 "github.com/influxdata/influxdb/v2/pkg/errors" |
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.
errors3
should be in a separate import block following the standard Go packages.
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.
Turns out GoLand was set to gofmt
import style rather than goimports
. Seems a violation of Go's whole ethos to have 2 different formatting styles.
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.
Been bitten by that many times....
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.
More import formatting.
@@ -16,6 +15,8 @@ import ( | |||
"sync" | |||
"time" | |||
|
|||
errors3 "github.com/influxdata/influxdb/v2/pkg/errors" |
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 thhink errors3
should be in the block below, not a separate block.
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.
It passed the lint stage (and my formatting once I set the right style), so I think this is right
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.
Weird.
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 am surprised by the acceptance of the import ordering, but if the tools like it...
Description
The current delete statement does not take into account the measurement predicate. It will blindly loop through all the measurements in each shard and run the delete. This can be very slow on instances with a lot of measurements. This change checks for measurement existence, and if found, only runs the delete on that measurement.
Context
The value add is faster deletes that have a measurement predicate.
Note for reviewers:
Check the semantic commit type: