-
Notifications
You must be signed in to change notification settings - Fork 115
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
chore: optimize single-record blocking write #297
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,7 +51,8 @@ import ( | |
// } | ||
type WriteAPIBlocking interface { | ||
// WriteRecord writes line protocol record(s) into bucket. | ||
// WriteRecord writes without implicit batching. Batch is created from given number of records | ||
// WriteRecord writes without implicit batching. Batch is created from given number of records. | ||
// Individual arguments can also be batches (multiple records separated by newline). | ||
// Non-blocking alternative is available in the WriteAPI interface | ||
WriteRecord(ctx context.Context, line ...string) error | ||
// WritePoint data point into bucket. | ||
|
@@ -80,18 +81,25 @@ func (w *writeAPIBlocking) write(ctx context.Context, line string) error { | |
} | ||
|
||
func (w *writeAPIBlocking) WriteRecord(ctx context.Context, line ...string) error { | ||
if len(line) > 0 { | ||
var sb strings.Builder | ||
for _, line := range line { | ||
b := []byte(line) | ||
b = append(b, 0xa) | ||
if _, err := sb.Write(b); err != nil { | ||
return err | ||
} | ||
if len(line) == 0 { | ||
return nil | ||
} | ||
if len(line) == 1 { | ||
ln := line[0] | ||
if ln[len(ln)-1] != '\n' { | ||
ln += "\n" | ||
} | ||
return w.write(ctx, sb.String()) | ||
return w.write(ctx, ln) | ||
} | ||
return nil | ||
var sb strings.Builder | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. a nit: this could be also optimized to avoid useless allocations of temporary buffers, it could Grow in advance to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes The level and depth of optimizations is up to you ... it was just a nit from me. @vlastahajek is the approver herein. |
||
for _, line := range line { | ||
b := []byte(line) | ||
b = append(b, 0xa) | ||
if _, err := sb.Write(b); err != nil { | ||
return err | ||
} | ||
} | ||
return w.write(ctx, sb.String()) | ||
} | ||
|
||
func (w *writeAPIBlocking) WritePoint(ctx context.Context, point ...*write.Point) 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.
empty lines are ignored by InfluxDB, adding extra new line is not required, it could simply
return w.write(ctx, line[0])
https://github.com/influxdata/influxdb/blob/11c00813f106062829759a97673c7c945d5859ca/models/points.go#L369
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 existing implementation assumes there are no terminating newlines on the passed records; it's necessary to add one when missing to keep the existing behavior. Also, not adding a newline causes the tests to fail, and keeps this consistent with what's produced by line-protocol, see influxdata/line-protocol#52.
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.
BTW: That existing behavior is verified by the tests, which is why it isn't the way you suggested....