Skip to content
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

Improve throughput of blocking writes to the same bucket #352

Closed
kbolino opened this issue Sep 2, 2022 · 1 comment · Fixed by #354
Closed

Improve throughput of blocking writes to the same bucket #352

kbolino opened this issue Sep 2, 2022 · 1 comment · Fixed by #354

Comments

@kbolino
Copy link

kbolino commented Sep 2, 2022

Proposal:

High-level: The throughput of blocking writes to the same bucket is limited by a broad-scoped mutex.

Lower-level: Remove the mutex from WriteAPIBlocking implementation entirely, or at least reduce its scope to only exactly where it's needed.

Current behavior:

The method writeAPIBlocking.write locks a mutex at the start and doesn't unlock it until done (see source). The mutex seems necessary to guard w.batching and w.batch but not w.service nor w.writeOptions which can't be changed after creation.

Desired behavior:

  1. Remove the mutex entirely: No easy way to do this without breaking API changes
  2. Reduce the scope of the mutex: Since the mutex is only necessary to guard implicit-batching–related parameters, it does not need to be locked for the entire method call.

Here's a possible solution for option 2:

func (w *writeAPIBlocking) write(ctx context.Context, line string) error {
	body := line
	func(){
		w.mu.Lock()
		defer w.mu.Unlock()
		if w.batching {
			w.batch = append(w.batch, line)
			if len(w.batch) == int(w.writeOptions.BatchSize()) {
				body = strings.Join(w.batch, "\n")
				w.batch = w.batch[:0]
			} else {
				body = ""
			}
		}
	}()
	if body == "" {
		return nil
	}
	err := w.service.WriteBatch(ctx, iwrite.NewBatch(body, w.writeOptions.MaxRetryTime()))
	if err != nil {
		return err
	}
	return nil
}

This reduces the lock time to only long enough to check w.batching (if batching is disabled) or to modify/extract the current batch (if batching is enabled).

Double-checked locking could also be used in tandem with "sync/atomic".Bool (from Go 1.19) which would avoid the lock entirely if batching is disabled.

Alternatives considered:

  • Use the asynchronous API instead: this complicates error handling. I think the async API is great for recording app metrics but not so much for batch writing offline time-series data to InfluxDB.
  • Use api.NewWriteAPIBlocking instead of Client.WriteAPIBlocking to purposely create multiple writeAPIBlocking instances, one per goroutine: this seems to go against the grain of the API and also creates many redundant Services as well
  • Use Go's standard HTTP client and only rely on InfluxDB client for line protocol formatting: this is certainly a possibility but is a lot more work

Use case:

  • Throughput when batch writing: the server can handle simultaneous writes to the same bucket from many sources, but the client bottlenecks to a single operation at a time
@vlastahajek
Copy link
Contributor

As Flush and Write* can occur in different go-routines, there must be a synchronization.
Even though the delay on a lock is negligent compared to the network delays, I agree that your proposal leads to more practical code.

vlastahajek added a commit to bonitoo-io/influxdb-client-go that referenced this issue Sep 9, 2022
vlastahajek added a commit to bonitoo-io/influxdb-client-go that referenced this issue Sep 9, 2022
vlastahajek added a commit to bonitoo-io/influxdb-client-go that referenced this issue Sep 12, 2022
vlastahajek added a commit to bonitoo-io/influxdb-client-go that referenced this issue Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants