ClickHouse / ClickHouse/clickhouse-rs

RFC: Breaking `Inserter` API Changes

Open
#421 3 comments 1 reaction 0 assignees View on GitHub
enhancement
Dominant language
Rust
Stars
559
Forks
172
Avg merge
1d 19h
Merged PRs (30d)
3

Description

### Use case

#### Writing to `Inserter` from multiple threads

Applications that are trying to maximize client-side batching might want to buffer data from multiple threads simultaneously. As of now, they'd have to wrap `Inserter` in a mutex which would force all writes to be serial.

#### Async flushes

When the `period` elapses, `Inserter` has no way to flush the batch asynchronously, i.e. without the user calling `.write()` or `.commit()`. This is because the timeout checks and flushing are done directly in these methods.

Additionally, because these methods have to "block" on finishing the current `INSERT` statement and receiving a response, it's not possible to begin the next `INSERT` statement until we get a response for the previous one, resulting in a stall in ingestion rate (#41).

#### `.commit()` method naming is misleading

ClickHouse doesn't have transactions (not technically; there's a preview feature for transactions in Cloud but it's not generally available or recommended yet), and data is generally inserted as soon as it's received server-side. `.commit()` should perhaps more accurately be called `.flush()` because all it does is ensure the data in the buffer is sent to the server and finishes the current `INSERT` statement and underlying HTTP request.

#### Enabling retries

Currently, when an `INSERT` statement fails in the middle of writing data, it's impossible to tell how much data was successfully flushed to the server and the client-side buffer is completely discarded when it could be buffered and sent with the next request instead (#212).

### Describe the solution you'd like

Move the `Inserter` state machine to a background task and send data chunks via a channel. (Alternatively, some sort of ring buffer? I don't remember there being a good high-quality crate for ringbufs though.)

Since channels are generally MPSC, `.write()` can change to take `&self`.

Deprecate or just delete `.commit()` and `.force_commit()`. Add `.flush()` as a replacement for `.force_commit()`. Threshold-based committing/flushing can be done asynchronously on the background task without an explicit call.

_Maybe_ add a `.compress_buffer()` call (#233)?

### Describe the alternatives you've considered

### Additional context

Contributor guide

Open the contributing guide

Research direction

Start by tracing Inserter and its write(), commit(), and force_commit() methods; the issue does not name files or tests. Review the existing state machine, timeout and flush behavior, and referenced issues #41, #212, and #233. Done would require an agreed API and background-task design, so this RFC needs maintainer direction before implementation.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend, databases
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.